Implement emitting of some project dependencies #17
Conversation
There was a problem hiding this comment.
Nice to see the main functionality moving forward 😄
Don't take this to imply that any of what you did here is somehow "wrong", but I have some thoughts on how the tests are going to work. When I was writing that first test, it occurred to me that it's kind of inefficient to have the plugin generate a data structure and write it out into TOML, only to parse the TOML back into a data structure again. So I was thinking of refactoring the plugin to separate the part that actually generates the pyproject data structure from the part that writes that out into a TOML file, and that way most of our tests could be unit tests on the former part, so they wouldn't have to go through the full complexity of rendering and then re-parsing the TOML. We could just keep a couple of tests that write out the TOML file to make sure that the file-writing part doesn't fail.
Though, now that I think about it, even if you're on board with that, it is very possible that the best way to move forward is to just merge this and then I can refactor the tests in a separate PR to show you what I mean. What do you think?
I had it in my brain that `sorted()` returned an iterator and thus this needed to be captured into a list. Wrong, it actually does return a list, so the extra `list()` is redundant. Credit: #17 (comment)
I had it in my brain that `sorted()` returned an iterator and thus this needed to be captured into a list. Wrong, it actually does return a list, so the extra `list()` is redundant. Credit: #17 (comment)
sjlongland
force-pushed
the
feature/emit-dependencies
branch
from
91f66fa to
bf4e849
Compare
There was a problem hiding this comment.
It turns out that the refactoring I had in mind above is a little more complex than I anticipated, so I think it makes sense to leave that for a separate PR. I'll make that PR when I have the code ready.
Just one last suggestion about the possibly-redundant comments, but I'm approving this regardless because that's not worth blocking the merge. I leave it up to you @sjlongland to decide whether to remove those comments or not, and then once you do, feel free to merge it yourself. (Or you can re-request review and I'll merge it if you feel better doing it that way.)
This simplifies the logic needed to parse `install_requires`, `setup_requires`, etc.
This hopefully should copy across the dependencies from the original project `setup.py`. Omit project dependencies if none are specified.
I suspect different versions of `tomlkit` may at times, return slightly differing outputs which, whilst syntactically equivalent, will trip up a simple string comparison. Instead, parse the reference and returned TOML documents, then compare those parsed object trees. This should be less sensitive to formatting differences whilst still catching issues when they arise.
I had it in my brain that `sorted()` returned an iterator and thus this needed to be captured into a list. Wrong, it actually does return a list, so the extra `list()` is redundant. Credit: #17 (comment)
sjlongland
force-pushed
the
feature/emit-dependencies
branch
from
bf4e849 to
6ecd03c
Compare
|
Hmm I'm using the app and it's not showing me what is blocking the merge now... Assuming my approval went through, feel free to merge this yourself @sjlongland, and if it didn't then I'll figure this out when I get back to my computer. |
|
Yeah, I think Github got its knickers in a twist over the comments that I removed, it didn't notice the suggestion was basically implemented and that the comment was now resolved. :-) |
This picks up
setup_requiresandinstall_requires, and puts those in the build dependencies and project dependencies TOML output.If
setuptoolsis already listed by the project, we avoid duplicating it. Dependencies are also "normalised" in alphabetical order, with duplicates removed.