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: support prettier 1/3 #231

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

Anavelyz
Copy link
Collaborator

@Anavelyz Anavelyz commented Mar 5, 2024

Pull Request description

Prettier is an opinionated code formatter, ensuring that all output code conforms to a consistent style.

Fixes #180

Prettier was added to the project and to the template.

Modified files:

  • For the project:

    • .pre-commit-config.yaml
    • .prettierignore
    • .prettierrc.yaml
    • .editorconfig
  • For the template:

    • src/scicookie/cookiecutter.json (and it has been reformatted)
    • src/scicookie/{{cookiecutter.project_slug}}/.pre-commit-config.yaml (and it has been reformatted)
    • src/scicookie/hooks/post_gen_project.py
    • tests/smoke/linters.sh

Pull Request checklists

This PR is a:

  • bug-fix
  • new feature
  • maintenance

About this PR:

  • it includes tests.
  • the tests are executed on CI.
  • the tests generate log file(s) (path).
  • pre-commit hooks were executed locally.
  • this PR requires a project documentation update.

Author's checklist:

  • I have reviewed the changes and it contains no misspelling.
  • The code is well commented, especially in the parts that contain more complexity.
  • New and old tests passed locally.

Reviewer's checklist

  • I managed to reproduce the problem locally from the main branch
  • I managed to test the new changes locally
  • I confirm that the issues mentioned were fixed/resolved

Copy link
Collaborator Author

@Anavelyz Anavelyz left a comment

Choose a reason for hiding this comment

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

@xmnlab Can you have a look at it, please?

@Anavelyz Anavelyz marked this pull request as ready for review March 5, 2024 20:32
Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Hi @Anavelyz!

It is usually recommended to make all the auto-formatting changes (prettier changes in this case) in a single commit. This commit's hash can then be added to .git-blame-ignore-revs file to ensure that the git blame is preserved. For example, see pybamm's .git-blame-ignore-revs file.

But this is totally up to us, if we want to follow this or not 🙂

Copy link
Member

@xmnlab xmnlab left a comment

Choose a reason for hiding this comment

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

not sure what happened .. but it shouldn't change indentation from 2 spaces to 4.
maybe you will need to update .editorconfig as well

this is what I am using for astx: https://github.com/arxlang/astx/blob/main/.editorconfig

or just maybe update the prettierc file to define the indentation space directly there.

@Anavelyz
Copy link
Collaborator Author

Anavelyz commented Mar 6, 2024

not sure what happened .. but it shouldn't change indentation from 2 spaces to 4. maybe you will need to update .editorconfig as well

Yes, .editorconfig has 4 spaces set:

indent_size = 4

Excuse me, I did not read the issue carefully. There it was specified

@Anavelyz Anavelyz requested a review from xmnlab March 6, 2024 18:48
@xmnlab
Copy link
Member

xmnlab commented Mar 7, 2024

Hi @Anavelyz!

It is usually recommended to make all the auto-formatting changes (prettier changes in this case) in a single commit. This commit's hash can then be added to .git-blame-ignore-revs file to ensure that the git blame is preserved. For example, see pybamm's .git-blame-ignore-revs file.

But this is totally up to us, if we want to follow this or not 🙂

thanks @Saransh-cpp to bring that up.

tbh I am not familiar with that .. give me a bit to check that ;)

@xmnlab
Copy link
Member

xmnlab commented Mar 7, 2024

@Saransh-cpp , I think that it makes sense. thank you again for bring that up.

so the workflow would be:

  • merge this PR without any change from the prettier
  • create a new PR that apply changes from prettier and merge that
  • create a new PR for .git-blame-ignore-revs

is that right?

if so ... @Anavelyz, let's do it together ... ping me when you have a free time for that

@Saransh-cpp
Copy link
Member

We can do all of this in a single PR (in 3 separate commits) if we "Create a merge commit" instead of "Squash and merge".

If we want to "Squash and merge," then yes, it will require 3 separate PRs.

@xmnlab
Copy link
Member

xmnlab commented Mar 7, 2024

thanks for the answer @Saransh-cpp ! appreciate that

@Anavelyz Anavelyz force-pushed the 180-support-prettier branch from c4593d8 to 6b1c364 Compare March 8, 2024 20:06
@Anavelyz Anavelyz changed the title feat: support prettier feat: support prettier 1/3 Mar 8, 2024
Copy link
Member

@xmnlab xmnlab left a comment

Choose a reason for hiding this comment

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

lgtm! thanks @Anavelyz

@xmnlab xmnlab merged commit e57579c into osl-incubator:main Mar 8, 2024
7 of 11 checks passed
Copy link

🎉 This PR is included in version 0.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

YurelyCamacho pushed a commit to YurelyCamacho/cookiecutter-python that referenced this pull request Dec 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

template: Add support for prettier
3 participants