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

try to fix the declared with not given #2285

Merged
merged 33 commits into from
Nov 2, 2022
Merged

try to fix the declared with not given #2285

merged 33 commits into from
Nov 2, 2022

Conversation

zysend
Copy link
Contributor

@zysend zysend commented Oct 25, 2022

No description provided.

@zysend
Copy link
Contributor Author

zysend commented Oct 25, 2022

try to fix #2275

puts declared(params)
puts declared(params, include_missing: false)
puts declared(params, include_missing: false, include_not_dependent: false)

case 1,2

params

{"device_config":{"rtsp":"fake_rtsp", "number":"fake_rtsp"}}

result

{"device_type"=>nil, "device_config"=>{"number"=>"fake_rtsp"}}
{"device_config"=>{"number"=>"fake_rtsp"}}
{}

case 3

params

{"device_type": "type1","device_config":{"rtsp":"fake_rtsp", "number":"fake_rtsp"}}

result

{"device_type"=>"type1", "device_config"=>{"number"=>"fake_rtsp"}}
{"device_type"=>"type1", "device_config"=>{"number"=>"fake_rtsp"}}
{"device_type"=>"type1", "device_config"=>{"rtsp"=>"fake_rtsp"}}

@zysend zysend closed this Oct 25, 2022
@zysend zysend reopened this Oct 25, 2022
@zysend zysend marked this pull request as draft October 25, 2022 07:46
@zysend zysend marked this pull request as ready for review October 25, 2022 15:01
@zysend zysend mentioned this pull request Oct 25, 2022
@zysend zysend marked this pull request as draft October 25, 2022 15:31
@zysend zysend marked this pull request as ready for review October 25, 2022 15:34
Copy link
Member

@dblock dblock 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 great, albeit a bit complicated. I don't have anything better to suggest, but consider some refactoring to make it a bit cleaner/better organized/.

  • I think include_not_dependent should be called something else, what exactly does it do? exclude anything that is not given (given is evaluated?) or anything that's inside any given block? So maybe evaluate_given or just given: true/false?
  • Will nested given work? Let's add some tests/open some more bugs?
  • This needs documentation, and CHANGELOG.

@zysend
Copy link
Contributor Author

zysend commented Oct 29, 2022

Thanks for your advice.

  • include_not_dependent is not logical and readable,so I choose the evaluate_given.

  • My previous tests covered too few cases, so I added more tests.
    Then I found a bug on the declared_params_with_scope and I think again if the way is really organized.
    Finally, I choose a new binding way between the attr and scope instand of the attrs and scope.
    The new way is cleaner, but maybe unsafely(changes declared_params directly) and slowly(A scope may run meets_dependency? many times).

@zysend
Copy link
Contributor Author

zysend commented Oct 29, 2022

  • Added documentation, and CHANGELOG.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Code looks good, some minor stuff below. Let's work on the documentation.

Regarding performance: this is only a problem when calling declared(params), right? So there's no regression during execution in most cases, so I am not worried. Let me know if I misunderstood.

CHANGELOG.md Outdated Show resolved Hide resolved
UPGRADING.md Outdated Show resolved Hide resolved
lib/grape/validations/params_scope.rb Outdated Show resolved Hide resolved
@zysend
Copy link
Contributor Author

zysend commented Oct 30, 2022

I changed the structure of @declared_params, it will be initialized when api is initialized and only used when #declared(params) is called.
if you do not set decared_params_scope=false when using #declared(params), @declared_params will behave the same as the old data structure.

Thanks for the review, I will revise it soon.

@zysend
Copy link
Contributor Author

zysend commented Oct 31, 2022

I revised the docs and modified the default value of evaluate_given(evaluate_given: false means that it does not evaluate given and will return all declared_params).

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Looks great! Let's fix the English a little in README and this is good to go!

README.md Outdated Show resolved Hide resolved
lib/grape/dsl/inside_route.rb Outdated Show resolved Hide resolved
@zysend
Copy link
Contributor Author

zysend commented Nov 2, 2022

Great advise and I have changed these.

@dblock dblock merged commit 0e727c1 into ruby-grape:master Nov 2, 2022
@dblock
Copy link
Member

dblock commented Nov 2, 2022

I merged the change, thanks for contributing @zysend !

@zysend
Copy link
Contributor Author

zysend commented Nov 2, 2022

Thanks for your advise.

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.

2 participants