Skip to content

Conversation

@pp-mo
Copy link
Member

@pp-mo pp-mo commented Nov 26, 2019

Replaces #3558
Following discussions there, I'm proposing to remove the original, public 'coord_comparison' function. Replaced with an augmented, private version for internal use only.

In offline discussions with @bjlittle @lbdreyer , we agreed this was always rather over-complicated and under-explained for a user public method. Plus, the key _CoordGroup class it relies on was always private anyway.
Since it was practically impossible to understand or use with confidence, without also reading the sourcecode, we decided to simply retire it rather than providing an enhanced public function (or functions).

@pp-mo pp-mo force-pushed the remove_coord_comparison branch from 157396c to b455d24 Compare November 26, 2019 15:05
@pp-mo pp-mo added this to the v3.0.0 milestone Nov 26, 2019
@stephenworsley stephenworsley self-assigned this Nov 26, 2019
@pp-mo
Copy link
Member Author

pp-mo commented Nov 26, 2019

Thanks @stephenworsley .
Made a change, hoping to address your issue.
Are you working on further suggestions, or was that all ?

@stephenworsley
Copy link
Contributor

@pp-mo Looks good now, I reckon this is good to merge.



def coord_comparison(*cubes, object_get=None):
def _dimensional_metadata_comparison(*cubes, object_get=None):
Copy link
Member

Choose a reason for hiding this comment

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

@pp-mo Awesome! 👍

My only suggestion here, before we commit to this, is questioning the name of this private function (all 30+ characters of it).

I'm just making the explicit point that it can be anything that we want - it's private. I'm all for meaningful, appropriate names, rather than those that are cryptic, abstract or obfuscated, but now is probably the time to suggest shorter alternatives? It is a bit of a mouthful, right? 😮

Any suggestions? Can I start the bun-fight with simply _metadata_compare?

Copy link
Member Author

Choose a reason for hiding this comment

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

@bjlittle questioning the name of this private function

We just missed the boat on this.

I get the point, but I couldn't think of anything more concise and still clear.
Probably the term DimensionalMetadata is at fault here, also being a bit of a mouthful.

I guess we could have _dm_comparison.
Do you want to propose a post-fix in separate issue ?

Copy link
Member

@bjlittle bjlittle Nov 27, 2019

Choose a reason for hiding this comment

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

@pp-mo @stephenworsley No worries. Naming a thing is the hardest part, strangely.

I recently asked @lbdreyer for an alternative name to _DimensionalMetadata.... but we came up blank 😕

This isn't a high priority, by any means, just wanted to raise it as an obvious point to consider. We can stick with what we've got, and change at a later date in a follow-up PR, if we have an epiphany 💡 with agreement

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