Skip to content
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

feat: Implement standardized formatting. Closes #4 #78

Closed
wants to merge 8 commits into from

Conversation

MarkKragerup
Copy link
Contributor

This PR implements;

  1. Recommended eslint configurations.
  2. A minimalistic prettier setup for code formatting.
  3. An addition to .gitignore to support jetbrains editors.

For readabiliy this PR does not:

  1. Run of "prettier --write ." on the project.
  2. Enforce formatting in the pre-commit hook.
    This can be a seperate PR.

@MarkKragerup
Copy link
Contributor Author

@nzakas @jesusprubio My suggestion on modernising the formatting in regards to issue #4 . Can you give feedback and/or merge? 😺

@MarkKragerup MarkKragerup changed the title Closes #4 Implement standardized formatting. Closes #4 Mar 28, 2022
@nzakas
Copy link
Contributor

nzakas commented Mar 28, 2022

I've already updated the repo to format with ESLint on precommit. Is there any benefit to adding Prettier on top of what's already on main?

@MarkKragerup
Copy link
Contributor Author

As far as i can see, it runs eslint --fix on pre-commit. And the config is not setup to provide very strict formatting. The minimal prettier config provided here should allow for better separation the formatting, while still enforcing a more similar layout between pull requests. It will also become two different commands, one to be run locally and one to act as enforceable Github actions (the linting).
I can remove prettier from the PR, but i think it will make the contribution process a lot more neat in the long run.

@nzakas
Copy link
Contributor

nzakas commented Mar 30, 2022

I don’t see adding Prettier as providing much additional value at this point. ESLint can enforce the most important parts on precommit. I’d rather stick with one tool right now.

Copy link
Contributor

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking about this some more, I’m 👍

I think your point about the audience of the package potentially being people with less JS experience is valid and we should do what we can to help them. Just left a couple notes.

package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@MarkKragerup MarkKragerup changed the title Implement standardized formatting. Closes #4 feat: Implement standardized formatting. Closes #4 Apr 7, 2022
@MarkKragerup
Copy link
Contributor Author

@nzakas i included the correct lint-staged and suggested changes. I also fixed all the problems arising from using eslint:recommended config (code cleanup) and re-wrote from commonjs to modules, to get the most pleasing and maintainable code quality. It should be very good for the future pull requests, so i suggest we merge this one in first.

@nzakas
Copy link
Contributor

nzakas commented Apr 8, 2022

@MarkKragerup it’s considered a best practice to keep each pull request narrowly focused on just one change. By bundling all of these changes together, it makes the decision all-or-nothing.

This pull request should just be about adding Prettier. If you’d like to propose switching to ESM, please open a separate issue or pull request.

@MarkKragerup
Copy link
Contributor Author

@nzakas 100% agree on the best practise. This PR was adding prettier and the recommended eslint config. When I changed the commands it didnt allow for linting problems. Many problems had to do with require. I bet it was due to a wrong config ln my part, but I eagerly changed to ESM. If you want, I will rewrite it again - but ESM is preferable in my eyes, and more typescript ready, should the occasion arise.

@nzakas
Copy link
Contributor

nzakas commented Apr 9, 2022

I’d say just start with the changes related to prettier and we can discuss ESM elsewhere.

@nzakas nzakas closed this in 78292e0 Apr 18, 2022
@nzakas
Copy link
Contributor

nzakas commented Apr 18, 2022

To save time, I updated this myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants