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

load config from pyproject.toml or setup.cfg #79

Merged
merged 5 commits into from Aug 27, 2022
Merged

load config from pyproject.toml or setup.cfg #79

merged 5 commits into from Aug 27, 2022

Conversation

akeeman
Copy link
Contributor

@akeeman akeeman commented Sep 25, 2020

Simple implementation for the currently missing functionality to load configuration from certain files:

to do:

  • pytests

@coveralls
Copy link

coveralls commented Sep 25, 2020

Coverage Status

Coverage decreased (-0.5%) to 98.803% when pulling bd4400c on akeeman:patch-1 into d43d8a7 on myint:master.

autoflake.py Outdated Show resolved Hide resolved
@akeeman
Copy link
Contributor Author

akeeman commented Sep 28, 2020

@hakancelik96 Please review again

@akeeman
Copy link
Contributor Author

akeeman commented Oct 6, 2020

Made the setup.cfg loader and possible other loaders simpler by parsing boolean values from string elsewhere and added correct syntax highlighting in README.rst.

@hakancelik96 Please review again

Note that the test failure is due to an unrelated feature break in Python 3.10.0a0

@akeeman
Copy link
Contributor Author

akeeman commented Oct 16, 2020

@hakancelik96 any thoughts?

@hakancelikdev
Copy link

@hakancelik96 any thoughts?

I am not the maintainer of this project, sorry. I just gave a little suggestion. I am developing a similar project named unimport

@akeeman
Copy link
Contributor Author

akeeman commented Oct 16, 2020

@hakancelik96 I see. Thanks for sharing! I'll have a look.

@myint Have you got any feedback/other thoughts? I think that not having the ability to load configuration from a file is a real showstopper nowadays.

@akeeman
Copy link
Contributor Author

akeeman commented Nov 19, 2020

@myint have you any feedback or time to merge?

@mboutet
Copy link

mboutet commented Aug 12, 2021

I'm sorry to ping you like this @myint. I know how open source can be energy draining and I definitely understand if you don't have time to look into this PR.

However, is it possible to know if I should go ahead and publish the changes of this PR under a different name on PyPI so that I can use it?

Thanks a lot!

@janosh
Copy link
Contributor

janosh commented Apr 3, 2022

Is there just 1 person with merge permission for this repo? Maybe time to assign others? Seems like a waste of work not to get this in.

@myint
Copy link
Member

myint commented Apr 10, 2022

@sigmavirus24 Any thoughts on who should be added as additional maintainers? I think the other PyCQA owners would have appropriate privileges now that I've moved this and docformatter to this org.

@sigmavirus24
Copy link
Member

@sigmavirus24 Any thoughts on who should be added as additional maintainers? I think the other PyCQA owners would have appropriate privileges now that I've moved this and docformatter to this org.

I haven't been involved in this project so I don't know which contributors should be invited. Other owners on PyCQA tend to not merge things across other projects they're not actively working on.

Are there other folks in PyCQA that have worked on these projects you'd like to invite?

Also, my response time is going to be super slow with a 3 week old that I'm caring for


Seems like a waste of work not to get this in.

On this topic, someone sent a pull request and did work, yes. I've not evaluated it, but just because someone did work, does not mean it should ever just be accepted or merged because they did it. Projects need to guard their boundaries more and more because maintainer burnout is an increasingly prevalant problem and throwing new people into the fire doesn't ever improve things.

All new feature work has to be carefully criticized else a project become so large, unruly, and complex that it's functionally unmaintainable.

@TylerYep TylerYep mentioned this pull request May 28, 2022
@janosh
Copy link
Contributor

janosh commented Jun 8, 2022

Fwiw, the changes in this PR worked flawlessly for me.

pip install https://github.com/akeeman/autoflake/archive/patch-1.zip
autoflake **/*.py

Did nothing at first (as expected) but after adding the following to setup.cfg, it removed unused imports and variables.

# setup.cfg
[autoflake]
in_place = true
remove_unused_variables = true
remove_all_unused_imports = true
expand_star_imports = true
ignore_init_module_imports = true

The only thing I noticed (which I think is fine) is that only snake case is allowed. Some tools use param case in setup.cfg but trying

# setup.cfg
[autoflake]
in-place = true
remove-unused-variables = true
...

raised 'in-place' is not a valid configuration option.

Copy link
Collaborator

@fsouza fsouza left a comment

Choose a reason for hiding this comment

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

HI @akeeman, thank you very much for contributing! Are you still interested in getting this merged? If so, can you update your branch and address the comment on six?

autoflake.py Outdated Show resolved Hide resolved
autoflake.py Outdated Show resolved Hide resolved
@akeeman
Copy link
Contributor Author

akeeman commented Aug 24, 2022

Rebased

@akeeman akeeman marked this pull request as draft August 24, 2022 11:12
@akeeman
Copy link
Contributor Author

akeeman commented Aug 24, 2022

Let me fix #79 (comment) soon...

@akeeman
Copy link
Contributor Author

akeeman commented Aug 24, 2022

Doesn't anyone know some configuration (file) abstraction layer? Because it's insane to write this logic over and over again for different libraries...

setup.py Outdated Show resolved Hide resolved
@fsouza fsouza marked this pull request as ready for review August 27, 2022 23:10
Copy link
Collaborator

@fsouza fsouza left a comment

Choose a reason for hiding this comment

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

Thank you very much for contributing. I'm prepping a release, so I went ahead and addressed some of the comments to get the PR ready to go!

@fsouza fsouza merged commit 4f139dd into PyCQA:master Aug 27, 2022
@akeeman akeeman deleted the patch-1 branch September 1, 2022 06:25
@kynan
Copy link

kynan commented Oct 8, 2022

@fsouza @akeeman I notice that the way this is implemented, values from config files take precedence over command line flags. That behaviour is surprising and at odds with the UNIX convention where the order of precedence is command line flags > environment variable > user config files > system config files. Was that a deliberate choice?

@fsouza
Copy link
Collaborator

fsouza commented Oct 9, 2022

@fsouza @akeeman I notice that the way this is implemented, values from config files take precedence over command line flags. That behaviour is surprising and at odds with the UNIX convention where the order of precedence is command line flags > environment variable > user config files > system config files. Was that a deliberate choice?

No, that one slipped. Sorry about that, let me send a fix.

@fsouza
Copy link
Collaborator

fsouza commented Oct 9, 2022

@kynan can you try #158?

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

9 participants