Closed exhuma closed 4 years ago
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.
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).
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.
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 😉
Oh... in the PR I didn't add any design decisions I took along the way. The important ones can be summarised as follows:
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.config.py
file to keep everything related together in one module.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.As you mentioned in a comment above, I have removed the --no-index
flag from pip install
. But I wonder if there was a specific reason for adding that in the first place. Might be worthwile having @RJ722 signing it off.
After a little shower-thought this morning I added a final change to the config object: Following the standard docs it should either return a valid expression or something enclosed in <...>
. Being a subclass of dict
, the repr returned a valid expression, but that expression would not result in an equivalent instance.
So I added these two lines making this so.
It has the added benefit of making any debugging/logging lines more explicit as well if they include the config object.
Thanks for the PR @exhuma! I am looking into the Windows wheel problem right now: I can't seem to recall why --no-index
was added, but if it works without that, hooray! \o/
Though, I'm trying to make the builds a little more cleaner, by unifying the way we install wheels on all the platforms. One of the problems I can remember is that windows doesn't support wildcard/glob pattern matching, so I cannot directly write pip install dist/*.whl
. I'll explore it more, run a bunch of experiments on my fork (as I don't have access to a Windows machine) and raise a PR. However, let's not keep that PR a blocker on this.
Out of curiosity, why do you build/install wheels on CI (rather than using e.g. tox) at all?
Out of curiosity, why do you build/install wheels on CI (rather than using e.g. tox) at all?
I think we wanted to test that building wheels works (especially on Windows). From my understanding, pip would silently fall back to installing from source if building the wheel fails. But maybe this belongs in @RJ722's upcoming pull request. I'd appreciate your feedback there on how to simplify the tests.
Concerning the latest changes, I just wanted to give a sign of life. I'm still here, but during normal office-weeks I don't get all that much time to work on this. I'll try to get it done on Sunday if I don't get around to it before. In any case, I'm with you with the requested changes. I already felt a bit iffy with the dict-subclassing, the rest is housekeeping so I don't have anything substantial to add to the comments :wink:
Sounds good, thanks for the update!
Should be good now. I left the review comments as "unresolved" for now. If you're okay with them if would be nice to mark them as resolved. It makes it also easier for me to spot things which need additional work ;)
Hmmm... the Python-2 tests fail in the CI pipeline, but running tox -e py27
(and simply tox
) works on my machine, including Python 2.7.
I addressed the test-regression of 3a5802d in d4e1543, but the CI pipeline still fails. What am I missing?
It fails with:
tests/test_config.py:6: in <module>
from unittest.mock import patch
E ImportError: No module named mock
on Python 2.7. That module has only been added to the stdlib in Python 3.3. Not sure why it would work with Python 2.7 on your machine to be honest (though I haven't tried locally).
@jendrikseipp @RJ722 Given that it's 2020, why not drop Python 2.7 support entirely?
@The-Compiler : I added the dependency in tox.ini And on my local machine it works as expected (using tox, in Python2.7). I don't understand why this is not pulled in by the pipeline.
Ah, sorry, I missed that bit. My first guess would be that virtualenv (and its included pip
) is older on CI and somehow doesn't understand the environment marker correctly. Maybe adding a pip install --upgrade virtualenv
around here would help?
(or perhaps add --upgrade
and virtualenv to python -m pip install coveralls setuptools tox wheel
to make sure e.g. tox is upgraded as well).
@jendrikseipp @RJ722 Given that it's 2020, why not drop Python 2.7 support entirely?
Yes, I think we should do this soon. I'd rather not do it in this issue though, of course.
Tests are passing now even in Python 2. I left the dependency in tox.ini
though. If someone clones the repo and runs tox it needs to be in there. So there's a tiny duplication regarding the mock
dependency.
But if there's plans to drop Python 2 anyway, this shouldn't be too much of an issue. It's fairly easy to drop this from the workflow once Python 2 is gone.
The only remaining thing I see is the discussion about the sentinel value. FWIW, this is a pattern I nicked from the stdlib.
Again, just a comment to show a sign of life. I won't be able to work on this this week. I'm getting ready to take some time off next week, so I'll be a bit silent for the coming days. But I'll get back to this as soon as possible.
Thanks for the notice!
With the latest push I also merged with upstream to take advantage of the Python 3 stuff and cleaned out the remainders from tox which I added in earlier commits.
Thanks for bearing with me :-)
No worries. It's amusing to be on the other side of pull requests for once. And seeing that there are people who are even pickier than me gives me some chuckles. Usually I'm the one with that kind of feedback, so I understand where you're coming from :smile:
Thanks a lot for all your work, @exhuma!
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...
No worries. This was a difficult feature to add, so I'm glad we managed to get it in.
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: