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

"strange" interaction with m_style and pre-commit #227

Open
Remi-Gau opened this issue Aug 5, 2021 · 13 comments
Open

"strange" interaction with m_style and pre-commit #227

Remi-Gau opened this issue Aug 5, 2021 · 13 comments
Labels
documentation Improvements or additions to documentation

Comments

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Aug 5, 2021

miss_hit complains about an invalid config file when running a hook but is fine otherwise

MISS_HIT Component affected

  • Style checker

Possibly other components: I have not checked yet

Your operating system and Python version

  • Linux
  • python 3.8

This happens on this "sandbox" repo: https://github.com/Remi-Gau/clean_code_style

This should help reproduce the issue:

git clone https://github.com/Remi-Gau/clean_code_style.git
cd clean_code_style

# install latest miss_hit and pre-commit
pip install -r requirements.txt
pre-commit install

# make some changes
mh_copyright --add-notice S2/*.m

# try to commit
git add --all
git commit -m 'add copyright'

This throws this error:

mh_style.................................................................Failed
- hook id: mh_style
- exit code: 1

miss_hit.cfg: error: config file contains errors
In miss_hit.cfg, line 10
| suppress_rule: "naming_parameters"
|                ^^^^^^^^^^^^^^^^^^^ error: expected valid style rule name
.: error: cannot find project root because the config file contains errors: please add a config file with the 'project_root' directive
MISS_HIT Style Summary: 2 file(s) analysed, 3 error(s)

But simply running mh_style works fine.

Not sure what is happening and I suspect this might be some weird interaction between miss hit and pre-commit

@florianschanda
Copy link
Owner

That rule (naming_parameters) was added quite recently, and that error is consistent with running an older version. Can you check please which version of miss_hit is used / is installed by the hook? It might not be the same (for whatever reason) as your system installed version.

@florianschanda florianschanda added the ON_HOLD Waiting for additional user information label Aug 6, 2021
@florianschanda
Copy link
Owner

Also, I tried running through your instructions above and I was able to commit:

flosch@fat-potato:~/clean_code_style$ git commit -m 'add copyright'
[main e59354e] add copyright
 3 files changed, 5 insertions(+)

Is there a missing instruction for pre-commit, or did it just silently work?

@florianschanda
Copy link
Owner

florianschanda commented Aug 6, 2021

I think I will add way to show the version (in addition to -v) in the generated output, to help debug issues like this.

florianschanda added a commit that referenced this issue Aug 6, 2021
Show version and continue as normal: helpful to debug CI issues.
@Remi-Gau
Copy link
Contributor Author

Remi-Gau commented Aug 6, 2021

Also, I tried running through your instructions above and I was able to commit:

flosch@fat-potato:~/clean_code_style$ git commit -m 'add copyright'
[main e59354e] add copyright
 3 files changed, 5 insertions(+)

Is there a missing instruction for pre-commit, or did it just silently work?

I think I had forgotten an instruction to run pre-commit install: I updated my original post.

@Remi-Gau
Copy link
Contributor Author

Remi-Gau commented Aug 6, 2021

That rule (naming_parameters) was added quite recently, and that error is consistent with running an older version. Can you check please which version of miss_hit is used / is installed by the hook? It might not be the same (for whatever reason) as your system installed version.

Hum... Whether I use a virtual environment or my base conda environment I get the same version. I must be messing something up but not sure what.

(env): mh_style -v
MISS_HIT 0.9.25

@florianschanda
Copy link
Owner

You could try changing your hook to invoke mh_style -v instead and just observe the version number produced. I think knowing precisely what is run is the first step in resolving this issue:

  • if the wrong version is used then we know there is some setup issue; perhaps something can be added to the docs to clarify how best to set up the hooks to avoid this
  • if the right version (which is quite unlikely) is used then I can start debugging

@florianschanda
Copy link
Owner

@Remi-Gau do you have any updates/news here to point me in the right direction?

@Remi-Gau
Copy link
Contributor Author

Remi-Gau commented Oct 2, 2021

hey
only getting back to this now as I am finally bumping miss_hit version on some of my main projects.

So the hook is not using the right version.

This is the version in my environment.

╰─(env) ⠠⠵ mh_style -v
MISS_HIT 0.9.28

If I use this hook with pre commit:

repos:

-   repo: local

    hooks:

    - id: mh_version
      name: mh_version
      entry: mh_style
      args: [-v]
      verbose: true
      language: python
      additional_dependencies: [miss_hit_core]

I get this:

mh_version...............................................................Passed
- hook id: mh_version
- duration: 0.06s

MISS_HIT 0.9.22

@Remi-Gau
Copy link
Contributor Author

Remi-Gau commented Oct 2, 2021

More confusion.

Outside of all my environments I get this

─remi at remi-XPS-15-9570
╰─○ mh_style -v
MISS_HIT 0.9.18-dev

My base conda environment gives me this

(base) ╭─remi at remi-XPS-15-9570
╰─⠠⠵ mh_style -v     
MISS_HIT 0.9.20

So I am not quite sure where the hook is getting its command from.

@florianschanda
Copy link
Owner

Ugh :(

I think, when I looked at this pre-commit beast, they did some "clever" stuff with installing things in some local environment; and that the way we configured it was not the official recommended one. And that didn't work because I didn't quite set up the mh repo structure right and it would have been a major pain to fix it.

@Remi-Gau
Copy link
Contributor Author

Remi-Gau commented Oct 4, 2021

Do you want me to open an issue on the pre-commit repo to ask for guidance?

@florianschanda
Copy link
Owner

I suspect we'll hear the same thing we got told last time: make the miss_hit repo conform by having only a single setup.py.

There is good reasons why I don't want to do that; because it makes developing and running MISS_HIT way harder (I'd have to mess around with pythonpath all the time).

But maybe there is a better way we can set up the hook; or at least force an update of the tool in the local checked-in version that pre-commit seems to use. I wish it could be made to just use the system installed one, but apparently that is not so easy :/

@Remi-Gau
Copy link
Contributor Author

Remi-Gau commented Oct 4, 2021

OK I see.

Will look at regular interval if I can find a way to set up the hook differently.

In the meantime I think I will just try to make sure I always use the latest version of miss_hit in all my environments.

@florianschanda florianschanda added documentation Improvements or additions to documentation and removed ON_HOLD Waiting for additional user information labels Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants