Fix normalized names #289
Merged
Fix normalized names #289
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
op1st-prow
bot
added
the
size/XXL
frenzymadness
force-pushed
the
fix_normalized_names
branch
from
f92ea86
to
ccea608
Compare
|
/lgtm |
|
@frenzymadness Will take a look today. |
|
Thanks for the quick response. I've been using a requirements.txt generated using poetry in the meantime, so don't rush to get this fixed on my account. Tried this out locally. It looks like the top-level dependencies are correctly grouped now, but their dependencies are still being incorrectly grouped. Copied terminal output for attempt. Started with generating the lock file from the pyproject.toml included in bug report and then tried installing packages in a separate environment with the command used by the s2i builder image. Generate the lock file from pyproject.toml and confirming test application successfully runs Check out pull request and install in separate environment Install from poetry lock file and try to run test application, observed exception |
|
New changes are detected. LGTM label has been removed. |
Flask, alembic and their dependencies should be in default. Pytest and its dependencies should be in develop.
|
Thank you very much for your help. It's really good to have somebody to review these changes and test them in real environments. I've added three new commits - first two are fixing the broken implementation for poetry groups basically improving how categories are handled and switching the implementation to normalized names of the packages. The next three commits contain changes for the test data. It turned out that we had some wrong tests there for quite some time. They are split into separate commits with explanations so it should be easy to review them one by one and check that the generated Pipfile* files correspond to what is in pyproject.toml/poetry.lock. |
|
Repeated the test in the previous comment, and I was able to run the test app with the environment created by micropipenv. poetry environment micropipenv environment |
|
Great. Do you have some time to check the changes here or at least the impact they have on tests? |
|
@frenzymadness Don't think I can do better than CI you already have set up. Are pull request approvals not limited to repo owners? |
The problem with tests and CI in general is that the results are compared with manually generated files and the bug you uncovered showed that we can have bugs in the generated files as well. |
|
All right. I'm going to merge it and do a new release. Thank you for the report and all the help with testing! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #288
@eganma-umich thanks once more for the report! Could you please take a look at the fix? It also contains test so you can just check the generated files.