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

box types not named consistently #220

Open
julie777 opened this issue Mar 18, 2022 · 5 comments
Open

box types not named consistently #220

julie777 opened this issue Mar 18, 2022 · 5 comments

Comments

@julie777
Copy link

I always prefer class/type naming to be "adjective noun" as opposed to "noun adjective", which some prefer because filenames group together. "Adjective noun" matches normal speech, such as "hand me the blue pen".

When I first looked at box types I had to take time to understand what they meant, because my first reaction what "what is different to name one camel_killer_box and another box_dots".

I suggest that names be change to:

  • conversion_box
  • default_box
  • dots_box
  • camel_killer_box
  • frozen_box
  • recast_box
  • intact_types_box
    and allow previous names as synonyms for a time, marked with a deprecated warning.

However, I wonder why you didn't subclass box instead. You are calling the above type arguments.
Sub-classing Box would also have the benefit of

  • making it more explicit that it is a different type with some additional behavior
  • allow parameters specific to the sub-class to be specific to the init for that class
    • it is clear from the documentation that supplemental box arguments are specific to the type of box

Would you accept a pull request for this?

@cdgriffith
Copy link
Owner

cdgriffith commented Mar 20, 2022

Hi Juile, thanks for the feedback!

For the subclasses first, the issue with that is you can have multiple types of boxes at once. For example can have a default dots box. So while it would be cleaner and probably even faster, it would require a lot of subclasses to achieve all combinations, i.e. DefaultDotsBox, DefaultCamelKillerDotsBox, etc...

For the naming suggestions, I agree what you suggest is cleaner.

The minimal concern I have for the transition period is that people that use it now might use the reversal as a record in their own Boxes to check it's type somehow. For example if they do Box(box_dots=True, dots_box=True)

The name changes will have to be changes between major versions, so would target this for Box 7 with full switch with Box 8.

@julie777
Copy link
Author

I think I get it now. If I just replace the word "type" with "feature" then the parameters to the constructor just enable additional features for a Box.

Given that they are parameters to Box, the features don't need box in the name. Box(dots=True, default=True), etc.
For any name changes the typical way is to add the new name and keep the old name for some period, and during that period emit a warning when the old name is used. Eventually you get rid of the old name. I think the python standard library has a process of deprecate for some number of releases and then remove.

@cdgriffith
Copy link
Owner

You are correct with the "feature" mindset, and same is true with not needing the box in the keywords for technical reasons. However they are necessary from a practicality standpoint.

As the idea is for Box to be as close to a dict replacement as possible, it means Box could be used like Box(**my_dict) or Box(**kwargs). Which means those arguments would be expanded as keyword arguments, and we want to avoid collisions with common words. dots and default are actually good examples of that, as many dictionaries might already have those words in them. Then in this scenario they would be ripped out and used as Box's initialization parameters and not added to the internal dictionary.

@julie777
Copy link
Author

I understand your point. A user can write

dict(sape=4139, guido=4127, jack=4098)

However I can't imagine why anyone would write

d = dict(**mydict)

when this is more compact and more understandable, and probably more efficient.

d = dict(mydict)

@cdgriffith
Copy link
Owner

I've thought about this some more with the upcoming Box 7 release, and I simply can't justify so many people making these updates to just for a cleaner overall naming convention. I may be swayed in the future, but it's not something I am comfortable doing at this point. Don't want to cause another left-pad incident!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants