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

seastar-json2code: extract Parameter class and cleanups #2120

Merged
merged 4 commits into from
May 2, 2024

Conversation

tchaikov
Copy link
Contributor

this change paves the road to #2082. so that we can store more
properties in the Parameter class in this python script and then
translate them into a C++ class which is used to verify and expose
the content and schema of a parameter. so that they can be used
in handler_base::verify_mandatory_params() in a follow-up
change.

Refs #2082

simpler this way. it also silences following warning:

```
visually indented line with same indent as next logical line [E129]
```

from pycodestyle, see also https://peps.python.org/pep-0008/#indentation.

Signed-off-by: Kefu Chai <[email protected]>
less repeatings this way.

Signed-off-by: Kefu Chai <[email protected]>
@tchaikov tchaikov requested a review from amnonh February 26, 2024 04:05
this change paves the road to scylladb#2082. so that we can store more
properties in the Parameter class in this python script and then
translate them into a C++ class which is used to verify and expose
the content and schema of a parameter. so that they can be used
in `handler_base::verify_mandatory_params()` in a follow-up
change.

Refs scylladb#2082
Signed-off-by: Kefu Chai <[email protected]>
@tchaikov
Copy link
Contributor Author

tchaikov commented Mar 7, 2024

@amnonh hi Amnon, could you help review this change?

1 similar comment
@tchaikov
Copy link
Contributor Author

@amnonh hi Amnon, could you help review this change?

query_param = Parameter(param)
if query_param.is_required:
required_query_params.append(query_param)
if query_param.enum is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

why the is not none?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amnonh i am able to parse your question. do you mean "when it is not None"? or do you mean "why shall we check if query_param.enum is not None"?

Copy link
Contributor

Choose a reason for hiding this comment

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

why not:
if query_param.enum:

Copy link
Contributor Author

@tchaikov tchaikov Apr 30, 2024

Choose a reason for hiding this comment

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

i see. i wanted to be very specific. as this property is None if "enum" is not in its definition, or otherwise, a list of string. an empty list should probably be handled in generate_code_from_enum(), and an error should be reported instead of being ignored. if i had more bandwidth, i would like to add typing annotations to this script.

Copy link
Contributor

@amnonh amnonh left a comment

Choose a reason for hiding this comment

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

beside the nitpick about the if statement, it looks good

@tchaikov
Copy link
Contributor Author

tchaikov commented May 1, 2024

@scylladb/seastar-maint hello maintainers, could you help merge this change?

@xemul xemul closed this in 8b876dc May 2, 2024
@xemul xemul merged commit 8b876dc into scylladb:master May 2, 2024
14 checks passed
@tchaikov tchaikov deleted the seastar-json2mode-data-model branch May 2, 2024 04:45
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