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

Force env extension to the end, before user #130

Closed
wants to merge 4 commits into from

Conversation

galou
Copy link
Contributor

@galou galou commented Mar 23, 2021

Signed-off-by: Gaël Écorchard gael.ecorchard@cvut.cz

Signed-off-by: Gaël Écorchard <gael.ecorchard@cvut.cz>
Copy link
Collaborator

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

Quick cleanup on the patch, but otherwise this looks good. Moving the logic into the core makes sense. It would be great to add a quick unit test for this logic too to make sure it doesn't break with any future refactoring.

setup.py Outdated
@@ -31,15 +31,15 @@

kwargs = {
'name': 'rocker',
'version': '0.2.3',
'version': '0.2.4',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comments here about changing the version and the unrelated changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't get it, the other version change was for the minor number but here I changed the patch version. Do you mean that I should let the version to 0.2.3?

Signed-off-by: Gaël Écorchard <gael.ecorchard@cvut.cz>
Copy link
Contributor Author

@galou galou left a comment

Choose a reason for hiding this comment

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

I have to admit that I don't know how to write a unit test to test that the env extension is one before the last when it's supposed to be the sole extension tested.

setup.py Outdated
@@ -31,15 +31,15 @@

kwargs = {
'name': 'rocker',
'version': '0.2.3',
'version': '0.2.4',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't get it, the other version change was for the minor number but here I changed the patch version. Do you mean that I should let the version to 0.2.3?

@tfoote
Copy link
Collaborator

tfoote commented Mar 26, 2021

I should let the version to 0.2.3?

Correct. Your patch may be bundled in with other patches and then a release made. The version number will be bumped during the release process and is separate from your changes.

Signed-off-by: Gaël Écorchard <gael.ecorchard@cvut.cz>
@galou
Copy link
Contributor Author

galou commented Mar 26, 2021

Reverted the version number and unrelated changes. Should I squeeze commits?

@tfoote
Copy link
Collaborator

tfoote commented Mar 30, 2021

Thanks for the cleanup.

I have to admit that I don't know how to write a unit test to test that the env extension is one before the last when it's supposed to be the sole extension tested.

To write the test, you'll need to invoke it with a couple of extensions enabled and check the ordering.

extending this test

self.assertEqual(active_extensions[0].get_name(), 'user')

Coverage should be decent with the following tests:

  • push user to last
  • push env to last except user
  • push env to second to last with user

@agyoungs
Copy link
Contributor

If #242 is merged, the method to get the 'env' extension loaded just before the 'user' extension will need to be modified

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.

3 participants