Box icon indicating copy to clipboard operation
Box copied to clipboard

box types not named consistently

Open julie777 opened this issue 2 years ago • 5 comments

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?

julie777 avatar Mar 18 '22 17:03 julie777

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.

cdgriffith avatar Mar 20 '22 17:03 cdgriffith

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.

julie777 avatar Mar 29 '22 02:03 julie777

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.

cdgriffith avatar Mar 29 '22 13:03 cdgriffith

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)

julie777 avatar Mar 29 '22 19:03 julie777

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!

cdgriffith avatar Jan 28 '23 03:01 cdgriffith