Skip to content

Conversation

amangoel185
Copy link
Collaborator

Fixes #233

Copy link
Collaborator

@LovelyBuggies LovelyBuggies left a comment

Choose a reason for hiding this comment

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

Could you add tests (three expection tests)?

@amangoel185
Copy link
Collaborator Author

Have added the tests, @LovelyBuggies.

@LovelyBuggies
Copy link
Collaborator

LGTM now.

if set(data_dict) != set(range(len(args), self.ndim)):
raise TypeError("All axes must be accounted for in fill")
raise TypeError(
"All axes must be accounted for in fill, you may have used a disallowed name in the axes"
Copy link
Member

Choose a reason for hiding this comment

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

Eventually I'd only do this if you have an axis with a disallowed name, but this is fine for now.

I'd eventually make the disallowed_names a global DISALLOWED_NAMES: Final[set[str]] = {...}, then we could see if there is any overlap between these, and only suggest the "you may have used a disallowed name in the axes" part if there is a chance of this - most users seeing this error probably are making normal mistakes, not using one of these special names

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh okay, we can make it global after this.

@henryiii henryiii enabled auto-merge (squash) July 6, 2021 15:27
@henryiii henryiii merged commit b22d4dc into scikit-hep:main Jul 6, 2021
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.

[BUG] Hist does not forbid axes names that are reserved keywords for fill
4 participants