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

Made behaviour of keep_at_least parameter predictable #68

Merged
merged 7 commits into from
Nov 13, 2023

Conversation

yma-het
Copy link
Contributor

@yma-het yma-het commented Oct 12, 2023

When i'm reading the name of param "keep-at-least", i expect that all of images will be accounted in this number, not only marked for deletion. This PR makes behavior of keep-at-least parameter more predictable.

@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (a45cd46) 93.27% compared to head (800b55a) 89.74%.
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #68      +/-   ##
==========================================
- Coverage   93.27%   89.74%   -3.53%     
==========================================
  Files           1        1              
  Lines         223      234      +11     
  Branches       54       57       +3     
==========================================
+ Hits          208      210       +2     
- Misses          7       15       +8     
- Partials        8        9       +1     
Files Coverage Δ
main.py 89.74% <66.66%> (-3.53%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@sondrelg sondrelg left a comment

Choose a reason for hiding this comment

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

This is looking promising. Would you mind adding a PR description and pinging me when it's 100%?

main.py Show resolved Hide resolved
main.py Outdated
def validate_org_name(cls, v: str, values: dict) -> str | None:
if values['account_type'] == AccountType.ORG and not v:
@field_validator('org_name', mode='before')
def validate_org_name(cls, v: str, values: Any) -> str | None:
Copy link
Member

Choose a reason for hiding this comment

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

Is there not a more specific type signature we can use here?

Copy link
Member

Choose a reason for hiding this comment

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

Is ValidationInfo correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah

Comment on lines +414 to +416
# If we got here, most probably we will delete image.
# For pseudo-branching we set delete_image to true and
# handle cases with delete image by tag filtering in separate pseudo-branch
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain this a bit more? What are pseudo-branches?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my understanding, this code looks a lot like if .. else.
So i've decided to make comment that helps understand the logic here.

@yma-het
Copy link
Contributor Author

yma-het commented Nov 1, 2023

This is looking promising. Would you mind adding a PR description and pinging me when it's 100%?

Done.

@sondrelg
Copy link
Member

sondrelg commented Nov 2, 2023

Cheers @yma-het, I'll take a look at this this weekend 👍

@yma-het
Copy link
Contributor Author

yma-het commented Nov 13, 2023

Any news here?

@sondrelg sondrelg merged commit 3d27e6a into snok:main Nov 13, 2023
2 of 4 checks passed
@sondrelg
Copy link
Member

Changes look good. Thanks a lot @yma-het 🙇 The new release should be out now. Let me know if you run into any issues

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.

2 participants