NREL / BuildingMOTIF

Building Metadata OnTology Interoperability Framework (BuildingMOTIF)
https://buildingmotif.readthedocs.io/
Other
48 stars 6 forks source link

Add `poetry run format` script #253

Closed TShapinsky closed 1 year ago

TShapinsky commented 1 year ago

replace removed makefile with python script which runs formatting.

executed by poetry run format

gtfierro commented 1 year ago

@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 commented 1 year ago

255 should fix the broken test you are seeing here

TShapinsky commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago
  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.