-
Notifications
You must be signed in to change notification settings - Fork 146
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
Issue #249 - Add strip to get() and getall() #260
base: master
Are you sure you want to change the base?
Changes from 1 commit
fbdb881
cbc6e89
02b09b0
4185e19
ff23fdb
37dfd36
cbe7042
43ba89f
7ce8789
b044b76
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -200,12 +200,15 @@ def re_first( | |
return el | ||
return default | ||
|
||
def getall(self) -> List[str]: | ||
def getall(self, strip: bool = False) -> List[str]: | ||
""" | ||
Call the ``.get()`` method for each element is this list and return | ||
their results flattened, as a list of strings. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the new option needs to be mentioned in the docstring. |
||
""" | ||
return [x.get() for x in self] | ||
data = [x.get() for x in self] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can just pass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. x is a Selector, at that point neither default nor strip exist anymore There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. About adding the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Aren’t you also adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That ended up looking harder than it should when I tried. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, you are changing Selector.get() to have Also if you have issues with typing I can hep, I'm working on improving parsel typing right now.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No, the purpose of those overloads is to show that the type of the return value depends on whether
I believe just adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Also, it seems a little unintuitive to be using the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait, you are right.
Yeah, it's probably unusual to write
Not so sure about default=, but maybe there are use cases where it's useful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can keep this as is, strip for Selector.get() is less useful and can be implemented separately. |
||
if strip: | ||
return [x.strip() if x else x for x in data] | ||
return data | ||
|
||
extract = getall | ||
|
||
|
@@ -217,13 +220,20 @@ def get(self, default: None = None) -> Optional[str]: | |
def get(self, default: str) -> str: | ||
pass | ||
|
||
def get(self, default: Optional[str] = None) -> Optional[str]: | ||
@typing.overload | ||
def get(self, strip: bool) -> str: | ||
pass | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this necessary? (I am not saying it is not) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it is, I just saw the overloads above this line and thought it was required
I removed it on the last push. I don't understand the whole overload completely, but I feel those 2 are also unnecessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is related to typing, when parameter types affect output type, but I do not think it applies here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, removing it broke the pipeline for type checks. No overload variant of "get" of "SelectorList" matches argument type "bool" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As you added a new argument to the method you need to add them to all overloads too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And those two overloads are needed because the method either can return None if default wasn't passed or it returns the default instead, so this describes the typing info more clearly. |
||
|
||
def get( | ||
self, default: Optional[str] = None, strip: Optional[bool] = False | ||
Gallaecio marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) -> Optional[str]: | ||
""" | ||
Return the result of ``.get()`` for the first element in this list. | ||
If the list is empty, return the default value. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new option needs to be mentioned in the docstring |
||
""" | ||
for x in self: | ||
return x.get() | ||
value = x.get() | ||
return value.strip() if strip and value else value | ||
return default | ||
|
||
extract_first = get | ||
|
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.
Let’s make it keyword-only.
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.
Same on
get
.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.
Should we apply this to
typing.overload
ofget
as well?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.
I tried adding it to
get
but then this would require some type check ondefault
entry to validate it is astr
, either way the behavior looks fine without it forget