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.

Sign up for GitHub

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

Jump to bottom

Issue-468 The display of "hammr quota list" collapses... #491

Closed
wants to merge 7 commits into from
Closed

Issue-468 The display of "hammr quota list" collapses... #491

wants to merge 7 commits into from

Conversation

locobastos
Copy link

@locobastos locobastos commented Feb 6, 2019 •
edited by ghost
Loading

...when disk usage "Consumed > Limit"

Issue-468 The display of "hammr quota list" collapses...
7f9bf1d 
...when disk usage "Consumed > Limit"

* Handle the case when the consumed disk usage is greater than the quota size limit.
* Respect PEP8 recommandations
* Extracted all possible code into functions
* Added many TestCases

  Fix issue #468
@ghost ghost requested review from AliceAlbano and lqueiroga February 6, 2019 17:01
@ghost ghost self-assigned this Feb 6, 2019
@ghost ghost added this to the 3.8.11 milestone Feb 6, 2019
@ghost ghost mentioned this pull request Feb 6, 2019
Closed
@ghost
Copy link

ghost commented Feb 6, 2019

The previous work is on the PR #472

AliceAlbano
AliceAlbano suggested changes Feb 7, 2019
View reviewed changes
Copy link
Contributor

@AliceAlbano AliceAlbano left a comment

Choose a reason for hiding this comment

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

Thanks for this commit, the refactor and new tests are really nice :)
I have only small comments about it.

Also there is a general issue in hammr at the moment, the build is passing but a test is failing (not related at all to your commit, you can take a look at the travis build if you want to know more about it). We need to wait for this issue to be fixed before merging your PR

hammr/commands/quota/quota.py Outdated Show resolved Hide resolved
hammr/commands/quota/quota.py Outdated Show resolved Hide resolved
hammr/commands/quota/quota.py Outdated Show resolved Hide resolved
tests/unit/commands/quota/test_quota.py Outdated Show resolved Hide resolved
tests/unit/commands/quota/test_quota.py Outdated Show resolved Hide resolved
tests/unit/commands/quota/test_quota.py Outdated Show resolved Hide resolved
tests/unit/commands/quota/test_quota.py Outdated Show resolved Hide resolved
tests/unit/commands/quota/test_quota.py Outdated Show resolved Hide resolved
Issue-468 The display of "hammr quota list" collapses...
164d3c1 
...when disk usage "Consumed > Limit"

* Handle the case when the consumed disk usage is greater than the quota size limit.
* Respect PEP8 recommandations
* Extracted all possible code into functions
* Added many TestCases

  Fix issue #468
lqueiroga
lqueiroga requested changes Feb 15, 2019
View reviewed changes
Copy link
Collaborator

@lqueiroga lqueiroga left a comment

Choose a reason for hiding this comment

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

To be honest, it's really difficult to find the fix as you made a refactor in the same commit. For me it will be better to make the refactor in another commit.

hammr/commands/quota/quota.py Outdated
elif quota.type == constants.QUOTAS_GENERATION:
text = self.print_plural_quota("Generation", quota) + self.get_quota_nb(quota)
elif quota.type == constants.QUOTAS_DISK_USAGE:
text = self.print_plural_quota("Disk usage", quota) + self.get_quota_nb_binary(quota)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why get_quota_nb_binary is used only for disk usage ? You changed the behavior for other quotas here.

Copy link

@ghost ghost Feb 20, 2019 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

Ok, I reverted some commits.
The current patch is only fixing the ISSUE #468 and nothing else.
I'll PR a second commit to fix a second bug then a third commit to refactor quota class.
Each PR will be linked to a github issue i'll create later.

@ghost ghost mentioned this pull request Feb 20, 2019
Open
lqueiroga
lqueiroga previously approved these changes Feb 21, 2019
View reviewed changes
Copy link
Collaborator

@lqueiroga lqueiroga left a comment

Choose a reason for hiding this comment

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

Ok for me. Just be careful to edit the commit message, there are some info no more valid

AliceAlbano
AliceAlbano previously approved these changes Feb 21, 2019
View reviewed changes
Copy link
Contributor

@AliceAlbano AliceAlbano left a comment

Choose a reason for hiding this comment

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

Ok for me too, you will need to merge the master inside your branch to solve the CI issue though

@ghost
Copy link

ghost commented Feb 21, 2019

PR message fixed.

@ghost ghost dismissed stale reviews from AliceAlbano and lqueiroga via 65db8a2 February 21, 2019 15:52
@locobastos locobastos closed this Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lqueiroga lqueiroga lqueiroga left review comments

@AliceAlbano AliceAlbano AliceAlbano left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.8.11
Development

Successfully merging this pull request may close these issues.
3 participants
@locobastos @AliceAlbano @lqueiroga