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

Prettier formatter #2018

Merged
merged 49 commits into from
May 3, 2024
Merged

Prettier formatter #2018

merged 49 commits into from
May 3, 2024

Conversation

Domejko
Copy link
Contributor

@Domejko Domejko commented Apr 22, 2024

Closes #2004

@github-actions github-actions bot added documentation Improvements or additions to documentation testing Continuous integration testing scripts Training and evaluation scripts labels Apr 22, 2024
@Domejko Domejko mentioned this pull request Apr 25, 2024
Copy link
Collaborator

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

Vast majority of these changes look good to me. I left comments on the remaining ones that concern me. We'll also need to mention prettier in docs/user/contributing.rst and add CI in .github/workflows/style.yaml, but I can do those if you don't feel comfortable. I wish prettier supported rst and ipynb, but it already supports a lot more formats than I expected, so I'm happy to see that.

.github/SECURITY.md Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
@adamjstewart adamjstewart added this to the 0.6.0 milestone Apr 25, 2024
@Domejko
Copy link
Contributor Author

Domejko commented Apr 26, 2024

We'll also need to mention prettier in docs/user/contributing.rst and add CI in .github/workflows/style.yaml

In docs/user/contributing.rst I assume that you would like to just mention that Prettier is used in CI to keep the files format consistent.
As it comes to .github/workflows/style.yaml it have been already added to CI but due to the size and number of changes in this PR you could missed it. Let me know are you satisfied with this implementation in CI or does some changes have to be made.

.github/workflows/style.yaml Outdated Show resolved Hide resolved
.github/workflows/style.yaml Show resolved Hide resolved
@adamjstewart
Copy link
Collaborator

In docs/user/contributing.rst I assume that you would like to just mention that Prettier is used in CI to keep the files format consistent.

And also how to use prettier yourself. Basically, add it to the "Linters" section and follow a similar structure as ruff/mypy.

@github-actions github-actions bot added the dependencies Packaging and dependencies label Apr 26, 2024
@adamjstewart
Copy link
Collaborator

We should also add a pre-commit hook: https://prettier.io/docs/en/precommit#option-2-pre-commithttpsgithubcompre-commitpre-commit

package-lock.json Outdated Show resolved Hide resolved
.prettierignore Outdated Show resolved Hide resolved
@Domejko Domejko requested a review from adamjstewart April 29, 2024 11:23
Copy link
Collaborator

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

All formatted files look good now, just need to fix a few problems in CI.

.github/workflows/style.yaml Show resolved Hide resolved
.github/workflows/style.yaml Outdated Show resolved Hide resolved
.github/workflows/style.yaml Show resolved Hide resolved
.github/workflows/style.yaml Outdated Show resolved Hide resolved
@adamjstewart adamjstewart changed the title Prettier formater Prettier formatter May 1, 2024
@Domejko
Copy link
Contributor Author

Domejko commented May 2, 2024

That's strange, when I was testing this on my fork I got

Run npx prettier . --check
Checking formatting...
All matched files use Prettier code style!

and here again it can't find it and need to install it.

@adamjstewart
Copy link
Collaborator

What if you run it as prettier instead of npx prettier? Does that help?

@Domejko
Copy link
Contributor Author

Domejko commented May 2, 2024

Unfortunately it have to be run through npx. The issue is that npm ci need to be run in directory where JSON package files are and there is no flag to provide nested path to them. So when we run it in requirements/ it install node_modules there and as we run check command from root it doesn't see modules there. Strange is this that it worked at first on my fork.

We might try to use npm ci with --prefix flag and absolute path to project root because when I tried with simple ../ it didn't work. But I think that using npm install instead with path to requirements/ will be the cleanest solution. Installation process with npm ci takes 393ms so even if it doubles it will be unnoticeable.

@adamjstewart
Copy link
Collaborator

I guess in order of priorities, I have:

  1. It MUST install a stable version from a lock file, it CAN'T just install the latest version
  2. I would prefer that we store these files in requirements if we can, but we could put them in the root if that isn't possible
  3. I would prefer that we don't install the same thing in CI twice

We could change the working-directory for every step of the pipeline so that installation and running happen in requirements and we pass it .. to run on the whole project. Kinda ugly, but probably works.

@Domejko
Copy link
Contributor Author

Domejko commented May 2, 2024

I have used and tested npm install requirements/ and everything works fine. I had to add to package.json "name": "torchgeo" since without it git wasn't happy and we could not clone prettier. It looks like this:

      - name: Installing prettier
        run: |
          npm install requirements/
          npm cache clean --force
      - name: List npm dependencies
        run: npm ls --all
      - name: Run prettier formatting
        run: npx prettier . --check

Wanted to ask also are you sure you don't want to put node_modules to .gitignore ? Because when contributors will start to use it not all of them will have it set up as global ignore. It's not IDE specific.

@adamjstewart
Copy link
Collaborator

node_modules should be in .gitignore, IDE-specific things should not.

.gitignore Outdated Show resolved Hide resolved
Copy link
Collaborator

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

Everything looks good to me now!

Thanks for all of your hard work on this. I have exactly 0 years of experience with node/npm, so I'm glad I have the chance to learn a new tool.

I'll merge this and make it a required test in CI. We may play around with the settings in the future, but for now they all seem like good defaults.

@adamjstewart adamjstewart merged commit bd9c757 into microsoft:main May 3, 2024
18 checks passed
@Domejko
Copy link
Contributor Author

Domejko commented May 3, 2024

My pleasure, I'm happy that I could contribute to this project and too me it was also a very good experience, I have learn new tools that for sure might come in handy in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Packaging and dependencies documentation Improvements or additions to documentation scripts Training and evaluation scripts testing Continuous integration testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add YAML formatter
2 participants