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

Implement support for a pyproject.toml config file #215

Merged

Conversation

exhuma
Copy link
Contributor

@exhuma exhuma commented Jul 12, 2020

Description

This change will read config values from a pyproject.toml file first, and then update those values with the CLI arguments.

Related Issue

See #164

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation in the README file.
  • I have updated the documentation accordingly.
  • I have added an entry in CHANGELOG.md.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@exhuma
Copy link
Contributor Author

exhuma commented Jul 12, 2020

I just noticed the failure on Windows. I'll check this once I have access to a windows box. I assume I'll just have to use an older version of the toml lib.

@exhuma
Copy link
Contributor Author

exhuma commented Jul 12, 2020

Okay.... so it seems toml is not available for pypy on Windows. Question: Considering that vulture is CLI-tool, aiding in development of other applications, is it really necessary to support pypy? It's an honest question. I don't mind investing more time in making this work on pypy+win as well (if really necessary).

@jendrikseipp
Copy link
Owner

I think this problem is caused by the Github Action using different installation options on Windows compared to Linux and MacOS (see .github/workflows/main.yml). My guess is we only need to remove the --no-index and --find-links arguments. The current command works on Windows because Vulture didn't have dependencies. I think @RJ722 wanted to look into this anyway, so feel free to ignore this for this pull request. But of course you can also try to fix the GitHub Action.

@exhuma
Copy link
Contributor Author

exhuma commented Jul 12, 2020

That makes sense. Tomorrow I'll be out of the week-end and likely programming again all day, and after a whole day of coding my batteries are usually running on empty. I'll see if I can squeeze some time in for this. Sounds like a trivial fix. Whoever gets to it first then I guess 😉

@exhuma
Copy link
Contributor Author

exhuma commented Jul 12, 2020

Oh... in the PR I didn't add any design decisions I took along the way. The important ones can be summarised as follows:

  • I'm assuming vulture will be run from the project root as CWD when looking for pyproject.toml. This may not be as flexible as it could be, but it keeps the code-base way simpler. black does a fairly complex lookup, and I don't think that this added code-complexity is worth the gain. flit on the other hand also assumes it will be run from the project root. I decided to go the "flit" way.
  • The argument parser was moved to the new config.py file to keep everything related together in one module.
  • I added type-hints because they don't hurt, and I've gotten used to them. Especially for nicer editor autocompletion.
  • I decided to subclass dict for the configobject to ensure backwards-compatibility with existing code, while still being able to leverage dict.update(). I started off with a slightly more cumbersome aproach first with a data-class (albeit not a "real" dataclass due to the Python-2 dependency). This would have given rich type-hinting, but would have introduced cumbersome code for "merging" the two configs (argparse & toml). So I opted for the dict aproach.

Copy link
Owner

@jendrikseipp jendrikseipp left a comment

Choose a reason for hiding this comment

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

The code looks good, but you said I'm allowed to nitpick :-)

Adding typing information is an interesting idea, but I wouldn't want to introduce it together with the other changes. Also, can you please remove the pylint comments? I think they distract from the actual code.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
vulture/version.py Show resolved Hide resolved
vulture/config.py Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tests/test_config.py Show resolved Hide resolved
vulture/config.py Show resolved Hide resolved
vulture/config.py Outdated Show resolved Hide resolved
vulture/config.py Outdated Show resolved Hide resolved
Copy link
Owner

@jendrikseipp jendrikseipp left a comment

Choose a reason for hiding this comment

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

We're almost there :-)

vulture/config.py Outdated Show resolved Hide resolved
vulture/config.py Outdated Show resolved Hide resolved
vulture/config.py Outdated Show resolved Hide resolved
tests/test_config.py Show resolved Hide resolved
tests/test_config.py Outdated Show resolved Hide resolved
vulture/config.py Outdated Show resolved Hide resolved
vulture/config.py Outdated Show resolved Hide resolved
vulture/config.py Outdated Show resolved Hide resolved
@jendrikseipp jendrikseipp merged commit d0062b6 into jendrikseipp:master Aug 19, 2020
@jendrikseipp
Copy link
Owner

Thanks a lot for all your work, @exhuma!

@exhuma
Copy link
Contributor Author

exhuma commented Aug 20, 2020

Thanks as well for bearing with me and for the merge. I just now wanted to remove the MISSING value when I saw you already merged it. It seemed trivial at first but the "point-in-time" when the defaults are assigned in (in from_dict) makes it harder than it seems to remove the MISSING value. There may yet be a simple solution, but I did not see it at first glance.

Also, sorry for my rant from yesterday. I had a bad day...

@exhuma exhuma deleted the feature/164-config-file-support branch August 20, 2020 06:05
@jendrikseipp
Copy link
Owner

No worries. This was a difficult feature to add, so I'm glad we managed to get it in.

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.

4 participants