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

[improvement] Optional arguments in method definitions default to None #14

Merged
merged 10 commits into from
Jul 30, 2018

Conversation

ahggns
Copy link
Contributor

@ahggns ahggns commented Jul 24, 2018

No description provided.

@ahggns ahggns requested a review from ferozco July 24, 2018 16:20
docs: |
List of strings to filter on.
type: optional<list<integer>>
param-type: query
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make one of these path parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are path parameters allowed to be optional? I get java.lang.IllegalStateException: Path parameters must be primitives or aliases when I try.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, ya path params can't be optional. I meant to say headers, oops

Copy link
Contributor

@ferozco ferozco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is so much better! Lets add some integration tests to make sure the code still can compile/execute

@@ -366,7 +366,7 @@ def _fields(cls):
_map = None # type: Dict[str, str]
_alias = None # type: StringAliasExample

def __init__(self, string, integer, double_value, optional_item, items, set, map, alias):
def __init__(self, string, integer, double_value, optional_item=None, items, set, map, alias):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is illegal, @ferozco are we allowed to sort optional parameters to the end of the list?

SyntaxError: non-default argument follows default argument

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya of course! we can group by optionality then lex sort them

@uschi2000
Copy link

+add a slightly more descriptive PR name and/or changelog entry, please.

@ahggns ahggns changed the title Default optional arguments [improvement] Optional arguments in method definitions default to None Jul 25, 2018
@ferozco
Copy link
Contributor

ferozco commented Jul 26, 2018

Lets follow up on conjure-verification with additional test cases for optional query parameters, then pick them up here before merging this in

@ahggns
Copy link
Contributor Author

ahggns commented Jul 30, 2018

@ferozco this is ready for another look

@@ -155,4 +166,18 @@ static Builder builder() {

}


class PythonFieldComparator implements Comparator<PythonField> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we dedupe this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were a couple bad options:
(1) Try to merge PythonField and PythonEndpointParam, I didn't like this because they have some slightly different behavior, though maybe they shouldn't.
(2) Build an interface PythonNamedMaybeOptionalEntity, doesn't have to be that silly but seemed overly specific to the workflow.
(3) Do nothing.

I chose (3), maybe one of those other options is less bad or there's a fourth superior thing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya i suppose you're right

@@ -488,7 +488,7 @@ def _fields(cls):
_bearertoken = None # type: Optional[str]
_uuid = None # type: Optional[str]

def __init__(self, num, bool, integer, safelong, rid, bearertoken, uuid):
def __init__(self, bearertoken=None, bool=None, integer=None, num=None, rid=None, safelong=None, uuid=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

def run_test_with_argument(index, is_blacklisted, service, method_name, value):
target_method = getattr(service, method_name)
target_argspec = inspect.getargspec(target_method)
target_param = [e
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

man python is crazy

@ahggns ahggns merged commit cc9c977 into develop Jul 30, 2018
@ahggns ahggns deleted the ah/optional-arguments branch July 30, 2018 14:44
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.

None yet

3 participants