-
Notifications
You must be signed in to change notification settings - Fork 3
Add Fuzzy Versions, to compare bytes <> socrata #2061
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5e602fc to
86a3045
Compare
86a3045 to
dacb0ed
Compare
| if not self.original or not other.original: | ||
| return False | ||
|
|
||
| # Direct string comparison (handles case differences) | ||
| if self.original.lower().strip() == other.original.lower().strip(): | ||
| return True | ||
|
|
||
| # Compare normalized versions | ||
| return self.normalized == other.normalized |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thinking about trying to trim down a little...
Could we declare original and normalized as typed class attributes? I don't see how this first if clause would ever be met.
Then, since we're not returning "exact match" or the like, just true either way, should we just compared the normalized versions? We've already computed them and they should be identical if the originals were identical. Meaning we could skip the second if clause as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm with you. Made it terse. For the first case (if you're referring to if not self.original or not other.original) it'd be when it resolves to None == None which want to be False.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow my reading comprehension is terrible today sorry
| version = self.original.lower().strip() | ||
|
|
||
| # Handle quarter notation (e.g., "25q1", "24q2") | ||
| quarter_match = re.match(r"^(\d{2})q([1-4])$", version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be q|Q? I feel like we see both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry! We've lowered at this point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed - though latest commit adds an explicit test case for that (there was something close before, but not quite) and changes the comment
| ("September 2025", "202509", True), | ||
| ("JUNE 2024", "24q2", True), | ||
| ("march 2025", "20250315", True), | ||
| ("Q1 2025", "january 2025", False), # Different months in Q1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry why aren't these probably equal?
edit: oh direction matters! if something is Q1 it could be any of 3 different months so we'd rather not say these're equalish

This modifies a few things:
This PR is mostly slopped. I took a shot at integrating this with our existing
dcpy.utils.versionscode, but came away thinking that will be a bigger endeavor (and probably refactor of that module).