Major tidy-up #137
Major tidy-up #137
Conversation
Let's make this readable! * Key names have had wrapping quotes removed, except for non-schema keys (i.e. part of the PHP language definition rather than the language schema) * Comment keys moved to multi-line strings for readability * Whitespace `(?x)` regexes have been cleaned up and formatted -- they had both actual whitespace and escaped whitespace! * Capture indicies collapsed for better readability Note: This file is functionally the same as it was before, this has just been a reformat and tidy
Can you please revert this change? All other grammars have every key wrapped in quotes. |
|
Well, it's not a requirement of CSON, and it actually makes it clearer what keys are part of PHP and what are part of the grammar schema. Reverting this would make it much harder, visually, to find the strings that one needs to edit. It also makes it easier for one to understand what the schema is just by looking, and how to use it. It shouldn't matter what the other grammars are doing -- they're doing it wrong -- this way is much friendlier for anybody wanting to examine or modify the grammar. I can update the other grammars if it's consistency you want :) |
|
This is delving into personal preference - but I personally like everything quoted, and you obviously like them unquoted when possible :). This has been suggested before on both language-javascript and language-go but a final decision was never reached. atom/language-go#68 (comment) |
|
We all have closely held preferences (tabs for sure), but just look at this screen-shot -- especially the minimap; this is just quantitatively better when it comes to readability. It's just a wall of green if it's all quoted.
You basically have domain over the language grammars, so it's up to you, but there's no point to this PR if we stick to the wall of green. I simply can't work with that, it's hell to comprehend the structure. :( |
|
After debating over this for a bit, I've come to the conclusion that unquoting the keys does make the CSON file easier to read. All the default language packages should be updated though to match. @atom/feedback any concerns, etc.? |
|
That's so amazing, thank you! This is going to make such a difference to people approaching the language files, they're a freighting prospect to edit. Would you like to me handle the other languages as I have done here? |
If no one raises any concerns in a few days, feel free to do so. |
This seems to be a hold-over from the XML/PLIST format from TM.
|
I'm 👍 for the unquoted keys. Thanks for looking at this!! |
|
As mentioned above, some keys have been intentionally left quoted - if this is going to be a thing, then it would be better if the rules for quoted/unquoted keys were documented somewhere. |
|
Hey @Kroc, any chance you can move all the unquoting stuff into a separate commit so I can review the other changes separately? Thanks :). |
|
I honestly have no idea how to do that! I'm not a strong git user. You could just search/replace the common keys I suppose? @Ingramz : The idea of that is to distinguish between the schema (allowed keys), and the user-defined-keys that acts as 'IDs' in the repository. This way when a reader sees -- edit: I can add comments to the top to describe the use of quoted/unquoted keys. Since all language grammars will be updated to match, there'll always be a guide in the right place. |
|
@Kroc I fully understood what you meant by it when you first mentioned it a couple days ago. Why I am asking this is because then it would be settled across the project once and for all in which way the grammar files should be formatted without having to rely on a person knowing it, making the new style easy for anyone to verify or follow. That solves the problem we are having here and the problem that others had in other repositories in a way that cannot be countered with "I like it that way". Also it would make it possible to extend on the current idea, if needed - add new rules or modify existing ones. If someone made a change to a grammar file, which wasn't styled correctly, it would be possible to easily point to the piece of text which defines the style instead of having to educate each person individiually. For motivation, see the following image posted in this stackexchange question. |
|
@Kroc Fair enough, it would be helpful in future PRs though :). |
|
@50Wliu OK, so for the other grammars, you want to submit the PR with the quotes intact for one commit, and then without afterwards -- or do you mean to submit the PR with the quotes and wait until you've merged it first? @Ingramz I had not thought on such a broad scale -- but remember too that there are developers like me who are going straight to these particular files without looking at any other part of Atom. Comments in the language grammars would be a starting point until some broader-scale decision could be made -- perhaps you could raise that issue in the right place? |
|
@Kroc The former. |
|
Ok, finally had some time to review this. Initial comments:
Regarding the first bullet point, the new formatting is not in line with our CoffeeScript formatting guidelines, and the new CSON file is currently producing 236 errors locally. Only one space can follow a colon. We should really set up Travis to lint CSON files as well... That's all I have for now. This is obviously all personal observations, so /cc @atom/feedback again. |
|
@50Wliu Thanks for taking a look; this is the first time I'm contributing to a large project, so please excuse my lack of understanding. On your points:
Ehhh, it's hard to say. Here's a comparison shot. I thought the tabs aligned nicer. It's more compact which makes it much easier to reference with the regex above.
For developers, like me, who go straight for the language grammar to make improvements these comments will reduce greatly the amount of reviewing and question answering you may have to field in the future -- everything the developer needs to know is right in front of them. Note that documentation on these grammars is already slim-to-none. The complete lack of direction on how to understand the file and what to do with it is an absurdly high barrier to entry. Only because I am very persistent did I not just give up and walk away, which is what I expect hundreds of developers have already done having faced a file with no comments and no direction on format / schema, not to mention 'the wall of green'. Central documentation is good, but that's a separate issue and I would urge that until such documentation exists and can be referenced in a comment, the grammar files should contain enough comments to get developers started. They do not have to be mutually exclusive, and updating the comments in the future is a very, very simple task.
Actually, that's what I've intended to do from the beginning, but I didn't want to push too many changes at once -- I'm just interested in making the leap from quoted to unquoted keys first and improvements like this one are an easy enough following step that I'm happy to tackle.
I didn't particularly see the need, but I have no objection to this at all.
I absolutely agree. All the regexes need a fine look, which I'm prepared to do should we be able to move to unquoted keys as this makes my life easier editing the file.
I was not aware of this -- which is exactly why a link to them should be in the comments so that you have less issues with PRs in the future. I'm happy to conform to the linter and can update the PR accordingly. Thanks for the continued direction and support! 👍 |
|
I'm still going to argue that the comments should be related to content and not how the file is parsed. In addition, having the same comments in each file is redundant when there could be a central location for it. A simple link in the README should suffice, and developers should definitely read the README before starting to work on a project 😉. |
|
Does anyone else think the To me, the |
Yep, seconded. That's how I structure all my grammars:
Or,
I'm also 1000% for the unquoted keys, because pointlessly quoting stuff is pretty pointless stuff. It's also possible the quoted keys were simply vestigial of the |
|
I think you mean And I would argue for |
Eh, I'm talking about my grammars, which're indented in a manner best described as, uh, idiosyncratic. Or calligraphic. Or "hideous", depending on how much you hate my indentation:
|
Let's make this readable!
(i.e. part of the PHP language definition rather than the language
schema)
(?x)regexes have been cleaned up and formatted -- theyhad both actual whitespace and escaped whitespace!
Note: This file is functionally the same as it was before, this has just
been a reformat and tidy