Skip to content

Add poetry run format script#253

Closed
TShapinsky wants to merge 2 commits intodevelopfrom
format_script
Closed

Add poetry run format script#253
TShapinsky wants to merge 2 commits intodevelopfrom
format_script

Conversation

@TShapinsky
Copy link
Member

replace removed makefile with python script which runs formatting.

executed by poetry run format

Copy link
Collaborator

@gtfierro gtfierro left a comment

Choose a reason for hiding this comment

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

Works great; thanks!

@gtfierro
Copy link
Collaborator

@TShapinsky I am so confused why that unit test is failing -- I'm pretty sure it is due to my last PR that I merged but my tests were passing in the branch. I'll take a look this week to see if I can figure out what is going on. I am confident that the changes in this PR have no relevance to the broken tests so I'm ok with this being merged in

@gtfierro
Copy link
Collaborator

#255 should fix the broken test you are seeing here

Copy link
Member

@MatthewSteen MatthewSteen left a comment

Choose a reason for hiding this comment

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

Thanks for doing this Toby...

  1. Could this be done instead with pre-commit run -a (currently it has --check args)?.
  2. The ci.yml and Developer Documentation should be updated to reflect any new workflows.
  3. I was trying to keep local and remote CI as similar as possible so results are deterministic. Thoughts?

@TShapinsky
Copy link
Member Author

Thanks for doing this Toby...

1. Could this be done instead with `pre-commit run -a` (currently it has `--check` args)?.

I'm not sure what would be accomplished by that. The problem that we're trying to fix isn't that pre-commit isn't running on all the files, it is that we don't have an easy way to format the files in the repo. It would be nice to figure out if there is a way to do that with an argument to pre-commit or something since this way will pick up files which aren't staged for commit.

2. The `ci.yml` and `Developer Documentation` should be updated to reflect any new workflows.

How were you anticipating this being involved in the ci.yml? this is just to assist in passing pre-commit checks

3. I was trying to keep local _and_ remote CI as similar as possible so results are deterministic. Thoughts?

The pre-commit and style checks should be unaffected by this, I believe.

@MatthewSteen
Copy link
Member

MatthewSteen commented May 30, 2023

We might be misunderstanding each other, so here's my attempt to clarify.


  1. Aren't the subprocess calls here nearly the same as what pre-commit-config.yaml is doing? If we remove the --check flags (as below), then we can just run pre-commit run -a to run the same formatting, which will also fix files (image from an isort fix). https://pre-commit.com/#pre-commit-run

I don't have a strong preference for pre-commit vs. the poetry script approach but think we should choose one over the other for clarity/simplicity. However, maybe I'm missing the value of pre-commit.

image

repos:
-   repo: https://github.com/pycqa/isort
    rev: 5.12.0
    hooks:
    -    id: isort
         entry: isort
-   repo: https://github.com/psf/black
    rev: 22.3.0
    hooks:
    -   id: black
        entry: black
-   repo: https://github.com/pycqa/flake8
    rev: 5.0.0
    hooks:
    -   id: flake8
        entry: poetry run flake8 buildingmotif
# can't poetry run becuase not present in repository https://github.com/pre-commit/mirrors-mypy
-   repo: https://github.com/pre-commit/mirrors-mypy
    rev: v0.931
    hooks:
    -   id: mypy
        args: ["--install-types", "--non-interactive", "--ignore-missing-imports"]
        additional_dependencies: [sqlalchemy2-stubs <= 0.0.2a20, SQLAlchemy <= 1.4]
exclude: docs/conf.py

@MatthewSteen
Copy link
Member

MatthewSteen commented May 30, 2023

  1. The ci.yml also runs isort, black, and mypy so I think the commands should be the same for local formatting and remote formatting so that the results are the same and we don't end up with local passes/remote failures. I also think this needs to be clearly documented for developers (which is what I started to do in developer_documentation.md) e.g...
    1. change code
    2. pre-commit run -a or poetry run format
    3. commit/push code
  2. Then, since the commands and files that are checked are the same when running locally and remotely, there shouldn't be any CI surprises.

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.

3 participants