Skip to content

Conversation

@HDembinski
Copy link
Member

No description provided.

@henryiii
Copy link
Member

henryiii commented Nov 6, 2019

We should provide wrappers here. Exposing the bare C++ objects gives us no flexibility in the future. The C++ version and the Python version will now have the same repr, rather than one having an upper case and one lower case. I thought that's the design we agreed upon? Your design?

@henryiii
Copy link
Member

henryiii commented Nov 6, 2019

This breaks code that worked in 0.5.2 and even was shown at PyHEP.

hist6 = bh.histogram(bh.axis.regular(10,0,10), storage=bh.storage.mean)

Now this is an error and not a warning.

We don't have that a huge number of users yet, but those we do should be respected for trusting us.

@HDembinski
Copy link
Member Author

hist6 = bh.histogram(bh.axis.regular(10,0,10), storage=bh.storage.mean)

That is your fault for introducing this in the first place.

@HDembinski
Copy link
Member Author

HDembinski commented Nov 7, 2019

I added storage values, which make

hist6 = bh.histogram(bh.axis.regular(10,0,10), storage=bh.storage.mean)

work and allow lazy people to skip the (). Same synatx without violating strong typing in Python. Now the value is snake_case and the class/type is CamelCase. There is a clear distinction between the two.

@HDembinski
Copy link
Member Author

By the way, we should have a test for this.
hist6 = bh.histogram(bh.axis.regular(10,0,10), storage=bh.storage.mean)
if we want to officially support it.

@henryiii
Copy link
Member

henryiii commented Nov 7, 2019

I do not like duplicated Int and int. I have very carefully, module by module, made sure the auto-completion list is clean and helpful without any extra items. What happened to "there should only be one way to do it"? Now you have two identically named storages, and one supports a () and one doesn't, only different by a capital letter. And if a user uses the cpp module, the identically named lower case item is now a type instead of an instance. I prefer your original solution, which I adopted in #196. I also have already told you that we are wrapping the bare objects (also your design).

@henryiii
Copy link
Member

henryiii commented Nov 7, 2019

You are not respecting the design or thought I have put into the Python bindings.

@HDembinski
Copy link
Member Author

I think we should officially support the shortcuts for storages which do not take arguments. I am not worried about the "clutter" of the auto-completion list of the storage module.

@HDembinski
Copy link
Member Author

You are not respecting the design or thought I have put into the Python bindings.

I accepted many changes that went well beyond my original vision. I think I showed you enough respect.

@HDembinski HDembinski merged commit 9330689 into develop Nov 7, 2019
@HDembinski HDembinski deleted the remove_support_for_passing_bare_storage_types branch November 7, 2019 17:05
@henryiii
Copy link
Member

henryiii commented Nov 7, 2019

It is important to me that we have a base class Storage for future type checking. I can implement a strong typing system later that can be checked in mypy, and used in IDEs and things to help users. You are undoing the wrapping I implement in response to #183. Which you happily claimed you were in favor of and that it was your idea. This is an unrelated change to int/Int.

@HDembinski
Copy link
Member Author

HDembinski commented Nov 8, 2019

I don't understand why we need a base class Storage. We need shims for those classes which actually use other classes to do the work, like histogram and the axis. But there is no shimming necessary for the storages. If you want to add a doc string in Python, I think this can be done by assigning some_storage.__doc__ = "foo"

@henryiii
Copy link
Member

henryiii commented Nov 8, 2019

A lot depends on your answer to the question at the bottom of #200. We need a parent class "Storage" so we can implement static type hints eventually and put a type in the doc strings, we need different __name__, __repr__ and __module__ in the cpp module. If you don't care about the cpp module correctly reporting names and things like that, then we can probably inject what we need and skip the shim.

@HDembinski
Copy link
Member Author

Ok, I see your point. I would be ok with having different __name__, __repr__ and __module__ for the cpp module, if it simplifies the implementation.

HDembinski added a commit that referenced this pull request Dec 1, 2019
* removing support for passing bare storage types
* adding preconfigured instances for lazy people
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