public-transport / hafas-client

JavaScript client for HAFAS public transport APIs.
ISC License
263 stars 52 forks source link

Update ESLint and add Stylistic Formatting #305

Closed KristjanESPERANTO closed 5 months ago

KristjanESPERANTO commented 7 months ago

Prettier and ESLint are a nice combination.

Update: Because prettier hardly allows any customization, we switched to stylistic in the process of the review.

socket-security[bot] commented 7 months ago

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@stylistic/eslint-plugin@1.6.0 Transitive: environment, eval, filesystem, shell, unsafe +118 55.6 MB antfu

View full report↗︎

KristjanESPERANTO commented 7 months ago

The tests are failing because I didn't run npm run lint:fix intentionally.

derhuerst commented 7 months ago

A appreciate the intention to make the code's formatting consistent and reproducible, but infortunately in the past years, whenever I have encountered prettier-formatted code, prettier's formatting style was really incompatible with the line-based nature of Git and trying to minimise unrelated changes in commits.

I'm aware that I'm letting technical limitations of Git dictate my code style here, but given that I'm bound to use Git, the benefits of having Git-diff-friendly code formatting are significant: Less conflicts while rebasing/merging, more meaningful git blame to remember why a change was made, etc.

The problem that I have with automated formatting: It depends on a specific code section's business logic how likely it is to be extended in the future (e.g. an another argument added to a function call, or another field to an object), and the current tools cannot estimate that. For example a addNumbers(a, b) call is unlikely to be extended in the future, a sendRequest(url, body, headers) much more.

If we accept a "dumb" and therefore inherently Git-diff-unfriendly code formatting tool, let's pick one – or a well-supported way to get prettier to do this – that does the following:

KristjanESPERANTO commented 7 months ago

Thank you for the detailed explanation! This gives me a new perspective and I'm thinking about adapting my usual workflow.

Since prettier has very few options, I have chosen a different approach with the last commit. stylistic is much more adjustable than prettier. What do you think, is it worth going further in this direction?

KristjanESPERANTO commented 7 months ago

As example I added the linted/formatted index.js.

KristjanESPERANTO commented 7 months ago

I see it as a learning process, don't worry.

Sorry, I just realized that I have to go back a few steps. Please wait with the review.

KristjanESPERANTO commented 7 months ago

I think I'm a bit closer to this what you want, but now I have to figure out why the tests are failing here. Locally they are not failing. I'll try to continue in the next few days.

KristjanESPERANTO commented 7 months ago

Okay, the unit tests are now running :rocket: and I think we are now close to your specifications. I'm delighted that stylistic is so customizable :smiley: There's certainly still potential for some fine-tuning.

Would you like to take a look? There are changes to only 171 files 😅

derhuerst commented 5 months ago

Because I couldn't find a better way to contact you, so I'll ask here, independently of this PR: What do you think about becoming a co- oder fallback maintainer (up to you which) in case I end up not being able to take care of hafas-client anymore? If you have more questions or comments on this, don't hesitate to contact me, e.g. via email!

derhuerst commented 5 months ago

Thanks!

KristjanESPERANTO commented 5 months ago

:partying_face: