-
Notifications
You must be signed in to change notification settings - Fork 42
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
refactor Python API to be more pythonic #267
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming and use of Python's property
seem reasonable to me.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hendrikmuhs LGTM!
I guess we might want to have another pass later on, and names of methods themselves, not just casing ... but this is a good first step.
@narekgharibyan do you have specific method names in mind? I think the |
Not yet ... one example that come to my mind is to skip So instead of I guess this is more python dict like ... but no strong opinion yet. |
makes sense. This is a python only addition anyway. I will rename to |
9f779b9
to
ba69c8c
Compare
I went another pass over the names and changed a few of them. I took the builtin re module as example:
LBNL, I removed the remaining Let me know what you think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -4,3 +4,4 @@ msgpack>=1.0.0 | |||
pytest>=7.1.1 | |||
twine>=4.0.0 | |||
wheel>=0.37.1 | |||
cython>=3.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems this file doesn't end with "empty line"
This PR introduces a way to change the API's into a more pythonic API. The old naming is still possible, but causes a deprecation message. I rewrote all Dictionary and most of the Match classes(except the attributes). Whenever possible I introduced properties.
I intend to rewrite the remaining API's asap in follow-up PR's.
I appreciate feedback about namings and the (correct?) use of properties.
/CC @iamkhav