default optional const parameter to none - #1171
Conversation
iscai-msft
left a comment
iscai-msft
left a comment
There was a problem hiding this comment.
overall looks really good, I think good to go once these comments are addressed!
| map_error(status_code=response.status_code, response=response, error_map=error_map) | ||
| error = self._deserialize.failsafe_deserialize("object", pipeline_response) | ||
| raise HttpResponseError(response=response, model=error) | ||
|
|
There was a problem hiding this comment.
do you know why there's different white space for mergepatchjson? Doesn't look like there were any code changes that would have led to it, maybe did you accidentally format it my opening the generated code?
There was a problem hiding this comment.
Be strange that this happen when I generate the test project in windows, and don't happen in Linux. So I committed the Linux result to undo the changes.
But I guess it's not related with this PR because this also happen in branch autorestv3 in my local machine. I will investigate further when have time.
| into your code flow. | ||
|
|
||
| :keyword input: Default value is "value1". | ||
| :keyword input: Default value is None. |
There was a problem hiding this comment.
Can we also update the documentation to say the other value is "value1"? I think playing with code here is good
iscai-msft
left a comment
iscai-msft
left a comment
There was a problem hiding this comment.
some last minute nits and changelog comments, otherwise looks good!
| into your code flow. | ||
|
|
||
| :keyword input: Default value is "value1". | ||
| :keyword input: Default value is None. Another value candidate is "value1". |
There was a problem hiding this comment.
nit: (feel free to not take). For enum values for example, we do "Possible values are...". For consistency I think it would be nice to do: "Possible values are 'value1' and None". Thoughts?
There was a problem hiding this comment.
Thanks Isabella, It's important suggestion that we should follow existing pattern, and I will move "Possible values are..." ahead of "Default value is .." like
|
|
||
| - Add default value consistently for parameters #1164 | ||
| - Make `content_type` param keyword-only if there are multiple content types #1167 | ||
| - Add flag `--default-optional-constants-to-none` for optional constant parameters #1171 |
There was a problem hiding this comment.
Should have picked this up in the first review, but we should do a minor version bump. We should add two entries, one under "New Features", and one other "Breaking Changes in Version Tolerant Generation".
In "New Features", we add this entry, saying "default optional constant parameters to None with this flag", and in "Brekaing Changes in Version Tolerant Generation" we say that by default we generate with optional constant parameters as None now by defaulting this flag to true
…nglong-liu/autorest.python into 20220225-optional-const-default-none
iscai-msft
left a comment
iscai-msft
left a comment
There was a problem hiding this comment.
looks great, thank you! I didn't actually end up releasing 5.13.0 yesterday, so I'll change the changelog later to move it to 5.13.0. Thanks again!
…into next_link_opaque * 'autorestv3' of https://github.com/Azure/autorest.python: Update package.json Update ChangeLog.md default optional const parameter to none (#1171) add storage to ci checks (#1176) fail on regeneration diff (#1173) Drop Python 2.7 (#1175) Add secrets test (#1174) bump testserver version (#1172)
…into inherit_customization * 'autorestv3' of https://github.com/Azure/autorest.python: Update package.json Update ChangeLog.md default optional const parameter to none (#1171) add storage to ci checks (#1176) fail on regeneration diff (#1173) Drop Python 2.7 (#1175)
…into mypy_code * 'autorestv3' of https://github.com/Azure/autorest.python: (26 commits) dpg service-driven-test (#1180) prepare for release (#1181) [Package mode] add new flag `--package-mode` (#1154) hide operation group docs (#1179) add coverage var (#1178) improve logging (#1177) don't reformat query params for dpg next link calls (#1168) Update package.json Update ChangeLog.md default optional const parameter to none (#1171) add storage to ci checks (#1176) fail on regeneration diff (#1173) Drop Python 2.7 (#1175) Add secrets test (#1174) bump testserver version (#1172) make content type keyword only if multiple (#1167) add default value info for params to docs (#1164) update release tests to hopefully get coverage report green (#1152) fix windows: clean the outputFolderUri (#1135) Update CODEOWNERS ...
2 participants
for issue #893