Dataframe column names autocompletion #223
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
|
💚 CLA has been signed |
sethmlarson
suggested changes
Jul 1, 2020
There was a problem hiding this comment.
Thanks for this change! Definitely will be appreciated by users :)
A couple of comments:
- The CLA service is reporting you haven't signed the CLA, if you have let me know cuz sometimes it's slow and needs a kick to register.
- You should run
nox -s blackento automatically format and lint your code - GitHub tip: if you write "Closes #" then that issue will be automatically closed once this PR is closed. I edited your comment above to do that.
eland/utils.py
Outdated
| """ | ||
| Ensure the given string can be used as attribute on an object instance. | ||
| """ | ||
| return isinstance(s, str) and re.search(string=s, pattern=r"^[a-zA-Z_]+[a-zA-Z0-9_]*$") and s.replace('_', '') |
There was a problem hiding this comment.
- Regex can be changed to
^[a-zA-Z_][a-zA-Z0-9_]*$to remove the backtracking potential but have same meaning. - Even though it's technically "slower" can we change the last condition to be
s.replace("_", "") != ""to read better?
Took me a few seconds to figure out what was being done there. :) Also what is it needed for?__is a valid attr name (not a good one but eh)
There was a problem hiding this comment.
- you're right on regex, the
+is actually useless - I didn't think
__was a valid attribute name but it is indeed :) so I removed the last condition
eland/dataframe.py
Outdated
| """ | ||
| Provide autocompletion on field names in interactive environment. | ||
| """ | ||
| return super(NDFrame, self).__dir__() + [ |
There was a problem hiding this comment.
Is this intentionally super(NDFrame, self) instead of super().__dir__()?
There was a problem hiding this comment.
I was writing in p2/3 compatible, but indeed unnecessary here since eland is designed for python3
|
jenkins test this please |
|
jenkins test this please |
sethmlarson
approved these changes
Jul 2, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #222