Skip to content

update ordered list syntax #4653

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 22, 2014
Merged

update ordered list syntax #4653

merged 1 commit into from
Dec 22, 2014

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Dec 14, 2014

Q A
Doc fix? yes
New docs? no
Applies to all
Fixed tickets

@timglabisch
Copy link
Contributor

👍

@javiereguiluz
Copy link
Member

Personally I'm strongly against this change. In my opinion this magic syntax only complicates things.

As @weaverryan said in another PR, this makes it much harder to read the source document. Moreover, it complicates referencing to each list point (e.g. "as explained in the fourth list item ...", you have to count items manually). Lastly, it could add errors when you change the list but not the related explanation (e.g. in the text you are referring to the fourth element, but you change the list, the numbers change magically and the text is now wrong).

Symfony is known for fighting magic with its code ... I think we should do the same with its doc.

@xabbuh
Copy link
Member Author

xabbuh commented Dec 16, 2014

Moreover, it complicates referencing to each list point (e.g. "as explained in the fourth list item ...", you have to count items manually).

I see that point. Though I think that referring to an item this way isn't the best practice at all. If you did that, other people modifying the list later need to be aware of the fact that the item is referenced in the text which can easily be missed leading to inconsistent articles.

The advantage of this syntax is that you keep the diff small when an item is removed from the middle of the list or is added in the middle of the list.

@wouterj
Copy link
Member

wouterj commented Dec 16, 2014

Symfony is also known for coding standards to prevent diff that don't belong to the actual change made (suffixing each array item with a comma and not outlining the => in an array are 2 of these standards).

@stof
Copy link
Member

stof commented Dec 16, 2014

Moreover, it complicates referencing to each list point (e.g. "as explained in the fourth list item ...", you have to count items manually). Lastly, it could add errors when you change the list but not the related explanation (e.g. in the text you are referring to the fourth element, but you change the list, the numbers change magically and the text is now wrong).

Keeping the explicit numbering would not avoid such errors, but only make them harder to understand (because the source would display the number you use): the generated HTML uses a <ol>, and so will not use the numbers written in the rST but sequential numbers even when the source is wrong about them.

I'm 👍 for this change

@weaverryan
Copy link
Member

Hi guys! I tend to agree with Javier - it's nice that this turns it into numbers, but that's sort of a shortcut I don't feel I need in my life :). BUT, I hadn't ever thought about how it was rendered - so as Stof pointed out, apparently if we mis-number manually, then a "4." might actually be the 5th li in the ol at get a 5 when rendered. TSo anyways, I don't feel too strongly, and most people seem to like this, so let's do it .

@weaverryan weaverryan merged commit ee06eec into symfony:2.3 Dec 22, 2014
weaverryan added a commit that referenced this pull request Dec 22, 2014
This PR was merged into the 2.3 branch.

Discussion
----------

update ordered list syntax

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | all
| Fixed tickets |

Commits
-------

ee06eec update ordered list syntax
@weaverryan
Copy link
Member

Oh, and most importantly - thanks @xabbuh for another PR that touches on a lot of areas. Cheers!

@xabbuh xabbuh deleted the fix-lists branch December 23, 2014 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants