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

DM-35818: Fix and refactor various actions #26

Merged
merged 15 commits into from Aug 16, 2022
Merged

DM-35818: Fix and refactor various actions #26

merged 15 commits into from Aug 16, 2022

Conversation

taranu
Copy link
Contributor

@taranu taranu commented Aug 3, 2022

No description provided.

@taranu taranu force-pushed the tickets/DM-35818 branch 2 times, most recently from 9ff7d57 to e31308f Compare August 3, 2022 19:25
@taranu taranu force-pushed the tickets/DM-35818 branch 3 times, most recently from b445684 to 27b6f3b Compare August 4, 2022 18:27
Copy link
Contributor

@fred3m fred3m left a comment

Choose a reason for hiding this comment

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

Just a few small comments and suggestions.

from .selectors import RangeSelector


class CalcBinnedStatsAction(KeyedDataAction):
Copy link
Contributor

Choose a reason for hiding this comment

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

Your mixed use of snake and camel case variables in this class is confusing. Please pick an animal and stick with it :). In the case of this package, it looks like camel case is the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I guess I don't know how to politely tell everyone else that they're wrong because the dev guide says snake_case should be the default in primarily new code, which this package definitely is (and analysis_drp was too). None of it should have been written in camelCase in the first place.

)

@cached_property
def name_count(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer countName (and the same with maskName, medianName, etc), but if you feel strongly its ok to leave as nameCount etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to stick with snake_case (someone can yell at me later about it). But this one I always find tricky. Our classes will always be CamelCase, and it seems reasonable to say camelCase = CamelCase() # variable. But when you have N different things of the same logical type, I find it more convenient to have them prefixed and sorted by that type. Unfortunately, I think both name_count and count_name are potentially ambiguous depending on the convention (is it the name of the count or the count of a name?), so... 🤷🏼

@@ -163,6 +164,34 @@ def setDefaults(self):
]


class RangeSelector(VectorAction):
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if maximum and minimum should allow for None values. For example, if maximum is None then this would act as a threshold selector, only setting a lower bound; and if minimum is None then this would act as a ceiling selector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a reason for not putting in defaults, but I couldn't remember it, so I added them.

requirements.txt Show resolved Hide resolved
@taranu taranu force-pushed the tickets/DM-35818 branch 5 times, most recently from 1b0cbd8 to af59bb8 Compare August 15, 2022 21:52
@taranu taranu merged commit fb92487 into main Aug 16, 2022
@taranu taranu deleted the tickets/DM-35818 branch August 16, 2022 17:14
@taranu taranu restored the tickets/DM-35818 branch August 17, 2022 17:08
@taranu taranu deleted the tickets/DM-35818 branch August 17, 2022 17:10
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.

None yet

2 participants