openstreetmap / id-tagging-schema

🆔🏷 The presets and other tagging data used by the iD editor
ISC License
155 stars 154 forks source link

Introduce shared, automatic code formatting #333

Open tordans opened 2 years ago

tordans commented 2 years ago

It would be a great help, if we had a shared code formatter config and documentation.

In https://github.com/openstreetmap/id-tagging-schema/pull/331 my Editor VS Code Reformatted all manner of things. I added a .prettierrc in https://github.com/openstreetmap/id-tagging-schema/pull/331/commits/23da8f4ba40540a6d2121a7d50633837e62ecf8a which made thinks better (https://prettier.io/docs/en/options.html).

matkoniecz commented 2 years ago

Should re re-format all files once

Yes, this prevents unexpected linter changes mixed with actual PR changes.

Should we enforce the formatter via a Github Action as an error?

That would make contributing harder (right now format is sufficiently clear that it is possible to contribute without running code)

Maybe have it as an info warning, but not blocking merge?

Or even apply the formatting changes via Github Actions as commit? That seems nice if reasonable to setup and maintain.

tyrasd commented 2 years ago

In general, I would not be opposed to add such a thing. But to be honest, right out of the box, I don't particularly like the idea of of automated linting fixes. Also, I didn't experience any big issues with mixed-up indentations or other code formatting issues lately. Did you (//edit ah, I see #331 now… :thinking: )? If anything what sometimes feels slightly annoying is that the order of the different "sections" in the presets/fields are not always the same (e.g. sometimes the name is on top, sometimes on the bottom). A consistent order of these properties could make the files easier to read. Are there code linting tools which can do such a thing as well?

Since this would probably touch and change a lot of files, and it's low priority at the moment, I added this for the to do list of a next major release (working title v4) for now.

1ec5 commented 2 years ago

The only potential issue I can think of is unreadable diffs caused by automatic reformatter like the one @tordans has installed. Messy diffs could make mistakes harder to spot in code review. GitHub has an “ignore whitespace” option, but I don’t think it handles cases like where an array gets flattened onto one line instead of one line per item.

tordans commented 2 years ago

But to be honest, right out of the box, I don't particularly like the idea of of automated linting fixes.

Those are two things. Prettier only changes formatting and makes sure your frontend code is formatted the same way across developers.

Linting would also be great, but a different tool/config.

More at https://prettier.io/docs/en/comparison.html

Also, I didn't experience any big issues with mixed-up indentations or other code formatting issues lately.

Most users use the web editor from what I see based on the branch names ("patch-1"). But I am under the impression that the frontend development "culture" pretty much switched to automated code formatting on file save with Tools like VSCode.


Even if we don't use it or even enforce it, it would still be helpful to have for people like me who have this system set up.