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

Patched for flake8 version 5 #14

Closed
wants to merge 3 commits into from
Closed

Conversation

wieczorek1990
Copy link

flake8 version 5 removes flake8.options.config.ConfigFileFinder therefore config file finding is broken.
Possibly a commit on flake8 would be required to move the tuple with config file names to a variable.
https://github.com/PyCQA/flake8/blob/main/src/flake8/options/config.py#L31

@wieczorek1990 wieczorek1990 changed the title Change flake8 dependency version. Bump version. Patched for flake8 version 5 Aug 1, 2022
@wieczorek1990
Copy link
Author

Guys at flake8 don't want us to monkey patch.

Either way I copied the method and extended it to include pyproject.toml in checking.

Is now flake8 version 5 compatible.

@wieczorek1990
Copy link
Author

@csachs Can you please review?

def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.project_filenames = self.project_filenames + ('pyproject.toml',)
# Copied from flake8 version 5.0.2.

Choose a reason for hiding this comment

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

Could we just wrap the function call instead of rewriting it?

I think flake8-pyproject did that; but I think this implementation does a better job of finding enclosing folders that contain a pyproject.toml file.

Copy link
Author

@wieczorek1990 wieczorek1990 Aug 2, 2022

Choose a reason for hiding this comment

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

Hey @schinckel the current implementation on flake8 is a method with hardcoded list of candidate filenames.
We could do something like:

def _find_config_file(path: str) -> Optional[str]:
    result = flake8.options.config._find_config_file(path)
    if result is None:
        result = ...
    return result

But... without extracting methods from original _find_config_file it is basically the same method with a different list containing only ours pyproject.toml. I wrote a patch about extracting this candidate filename list, but it wasn't accepted.
In what way do you want to wrap the function call?
I don't want to change the logic of the original function.

Choose a reason for hiding this comment

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

Yeah, that was what I was thinking about: but we'd possibly need to run our finder first.

Copy link
Author

Choose a reason for hiding this comment

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

The finder code will be the same, equals duplication again.

@johnthagen
Copy link
Contributor

For reference, the rejected related upstream patch:

@DaanRademaker
Copy link

Is this ready to be merged?

@johnthagen
Copy link
Contributor

johnthagen commented Aug 8, 2022

Not a lot of public GitHub activity lately: https://github.com/csachs

@csachs Would you consider adding another developer or two as maintainers?

It's within his right to disappear from the project, so we may need to consider a friendly fork with a robust set of maintainers who have time to be responsive (especially given how upstream Flake8 changes could break this project at any time).

I reached out to Christian over email to see if he has to time to help with this. I want to avoid spamming him too much, so maybe after some time if there is no response (which is totally within his rights) the community can discuss a friendly fork.

@csachs
Copy link
Owner

csachs commented Aug 8, 2022

Sorry for the absence, I'm quite busy lately with my job (and had some vacations away from the computer).
I merged two smaller things and will take a look at this PR later.
Concerning maintainers, I'd happily add some other people, I'll just have to set the infrastructure in place so that they could release new packages to PyPI as well.
Who would want to help in maintaining?

@johnthagen
Copy link
Contributor

I'm quite busy lately with my job

No worries! We all understand.

I'd be willing to be a maintainer to help with small fixes and getting releases onto PyPI.

My PyPI account: https://pypi.org/user/johnthagen/

@johnthagen
Copy link
Contributor

@wieczorek1990 The README currently says:

while solving its task (with currently around 40 lines).

With this patch, it's now 67 lines of code. Perhaps we should simply remove the number of lines from the README?

@wieczorek1990
Copy link
Author

wieczorek1990 commented Aug 8, 2022 via email

@csachs
Copy link
Owner

csachs commented Aug 8, 2022

So, now I've read up the issue, PR and referencing PRs/issues a bit to get a better view of the situation.
Obviously such a change, in particular if it makes the formerly rather straight-forward monkey patching more difficult, is a sad occurrence for this little wrapper.

I'm sorry for not having reacted earlier, this package has gotten more popular in the past months than I had anticipated.

Thank you @wieczorek1990 and @johnthagen for looking into this.

As a first step, I've cherry picked c749077 to main, so there is a working, flake8 4.x based version on PyPI.

Concerning the way to address the changes, I'm somewhat reluctant of copying over the whole function; on one hand, it raises the need to stay in sync more closely, and on the other may introduce the need to specifically act on flake8's license terms if non-trivial amounts start to get copied (which is not really an issue, since it's a MIT license,, but just something to keep in mind).

Therefore my proposal would be to perform the monkey patching a bit rougher by modifying the AST.
Please check #15 .

(If the general opinion gravitates more to copying the function, we can look into this as well.)

jmmshn added a commit to materialsproject/pymatgen-analysis-defects that referenced this pull request Aug 17, 2022
@wieczorek1990
Copy link
Author

I prefer the AST patch version.

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.

None yet

6 participants