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

refactor Python API to be more pythonic #267

Merged
merged 10 commits into from
Jan 27, 2024

Conversation

hendrikmuhs
Copy link
Contributor

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

Copy link

@iamkhav iamkhav left a 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!

Copy link
Member

@narekgharibyan narekgharibyan left a 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.

@hendrikmuhs
Copy link
Contributor Author

hendrikmuhs commented Jan 9, 2024

@narekgharibyan do you have specific method names in mind?

I think the Match object is still a bit messy and could be simplified, but that entails removal not renaming. Renaming should probably be in sync with C++ changes.

@narekgharibyan
Copy link
Member

@narekgharibyan do you have specific method names in mind?

Not yet ... one example that come to my mind is to skip get_ prefix.

So instead of get_keys() to have keys().

I guess this is more python dict like ... but no strong opinion yet.

@hendrikmuhs
Copy link
Contributor Author

Not yet ... one example that come to my mind is to skip get_ prefix.

makes sense. This is a python only addition anyway. I will rename to items(), keys() and values().

python/src/pxds/dictionary.pxd Outdated Show resolved Hide resolved
python/src/pxds/dictionary.pxd Outdated Show resolved Hide resolved
python/src/pxds/dictionary.pxd Outdated Show resolved Hide resolved
python/src/pxds/dictionary.pxd Outdated Show resolved Hide resolved
python/src/pxds/dictionary.pxd Outdated Show resolved Hide resolved
python/src/pxds/dictionary.pxd Outdated Show resolved Hide resolved
python/src/addons/Dictionary.pyx Outdated Show resolved Hide resolved
python/src/pxds/match.pxd Outdated Show resolved Hide resolved
python/src/pxds/match.pxd Outdated Show resolved Hide resolved
@hendrikmuhs
Copy link
Contributor Author

@narekgharibyan @iamkhav

I went another pass over the names and changed a few of them. I took the builtin re module as example:

  • match, match_fuzzy, match_near for everything that must match from position 0
  • search, search_tokenized for everything that can match things in the whole input text.

LBNL, I removed the remaining get_ , e.g. statistics, manifest, ...

Let me know what you think.

Copy link
Member

@narekgharibyan narekgharibyan left a 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
Copy link
Member

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"

@hendrikmuhs hendrikmuhs merged commit 49b8c57 into KeyviDev:master Jan 27, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants