Closed dmeehan1968 closed 2 years ago
KrishnaIyer
commented
2 years ago Our formatter is indeed subjective but we use the same formatter across our codebases and this is the convention we follow. We may change the formatter or rules later but we don't plan to do this now.
The file should not be touched by validation. This seems counter-intuitive as it would break versioning.
How does this break versioning? YAML ignores new lines so the formatted and unformatted versions are equivalent.
dmeehan1968
commented
2 years ago Our formatter is indeed subjective but we use the same formatter across our codebases and this is the convention we follow. We may change the formatter or rules later but we don't plan to do this now.
The file should not be touched by validation. This seems counter-intuitive as it would break versioning.
How does this break versioning? YAML ignores new lines so the formatted and unformatted versions are equivalent.
As I showed in my example, newlines that I insert for clarity are perfectly acceptable to YAML, but the reformatting done by the fmt command break each array element onto its own line, which for larger arrays makes it hard to follow.
It breaks versioning because if I commit the original to version control, then run the fmt command, the files are modified. I could then create a pull request with outstanding 'silent' changes which would then be reformatted by subsequent submitters. Whilst it doesn't change the 'meaning' of the code, it does change the legibility. Note that this affects the JS files as well, as they are also subject to the fmt command (although I've so far no detected any changes to my modules, so the rules might be more relaxed).
I don't see why you feel the need to reformat submitted files when they are already valid YAML. No one gets to see these files, beyond the original submitter who cares about legibility, and other submitters for whom it could introduce changes to their fork that are of no consequence to them. It seems like an unnecessarily pedantic.
Please note, I'm not suggesting that you shouldn't enforce certain standards on the presentation of files, but in this instance (formatting of array elements when a certain, arbitrary and hidden, count is exceeded). It may be that the prettier command is forcing this on you (i.e. its not controllable through settings), but it seems unhelpful if it leads to obfuscation of code intent and thus reliability. If I choose not to submit the formatted code, I create a problem for others, and if I submit the formatted, I create a problem for myself. I'd ask you to consider whether this is 'worth it'.
KrishnaIyer
commented
2 years ago Ok so there are two things here.
I'm not a JS expert and this linter was chosen by our JS team. We use prettier not only here but also in The Things Stack Repo and other internal repos. Every repository has its own conventions and we use prettier for formatting JS/YAML/JSON files.
If enough users of this repo want the files to be formatted in a different way, we can look at it.
It breaks versioning because if I commit the original to version control, then run the fmt command, the files are modified. I could then create a pull request with outstanding 'silent' changes which would then be reformatted by subsequent submitters.
Our repository expects that you run the formatting and validation before you create a pull request: https://github.com/TheThingsNetwork/lorawan-devices#contributing
If you happen to use some other repository for your internal maintenance, the above steps have to be run for pull requests targeting our repository regardless of the specifics of prettier. So the fact that you commit a differently formatted code to your repository means that you have to run our formatter/linter when targeting our repo and this is part of the workflow.
Summary
I've create a code YAML file for a device, in which the byte representation can grow quite large.
The array syntax ([ and ]) extends beyond some number of items.
When the file is processed with
make validate fmt, the file is modified to break each element of the array across multiple lines (still using the [ and ] syntax, rather than YAML-syntax for each element. Comments between elements are preserved.What do you see now?
In this example, I have put line breaks after each 8 array elements, and in my code I have something similar with comments between lines so that I can remind myself of the byte order/purpose.
What the
fmtcommand does it decide to split every array element onto its own line, although comments are preserved.Before
make validate fmt:After
make validate fmt:...
What do you want to see instead?
The file should not be touched by validation. This seems counter-intuitive as it would break versioning.
It seems that this is specifically what the
fmtargument implies, but I don't see any reason to make this part of the functionality. If YAML files are valid (and passvalidate) then why do this?It's not clear what induces this, some shorter arrays don't experience it. The
prettiercommand (run as a result of using thefmtarg) has a default 80 column wrap which may or may not be the cause. It seems to be the number of elements in the array that matters, not the original formatting of the input YAML.Whilst 'I' could choose to skip the
fmtpass, one assumes that the pull request has this run automatically, thus changing my submission. Even if this is not the case, anyone else runningfmton their own fork will cause my submission to be changed, and potentially incorporated into their own pull request (if they don't spot it).It should be noted that
fmtalso processes javascript files. I really do not see a purpose to this. Why does theprettiercommand use the--writeoption? It appears that its principally being used for validation of the YAML files. Modifying the input files seems problematic....