-
-
Notifications
You must be signed in to change notification settings - Fork 25
Add excluded and mode attributes to Parameter #591
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
Add excluded and mode attributes to Parameter #591
Conversation
andrcuns
left a comment
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.
@ivan-neverov-al Thanks, changes look good to me, few minor questions from me.
|
@andrcuns missed one more rubocop fix |
andrcuns
left a comment
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.
It's much better now, but I think it's better to have explicit constants for each mode as it is more explicit and avoids using plain strings in code, better for future maintainability
Co-authored-by: Andrejs <[email protected]>
Co-authored-by: Andrejs <[email protected]>
Co-authored-by: Andrejs <[email protected]>
Co-authored-by: Andrejs <[email protected]>
Co-authored-by: Andrejs <[email protected]>
8ed36b9 to
1bbc954
Compare
andrcuns
left a comment
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.
One small thing I missed. I think we should not allow to create json with invalid values, it will cause issues downstream I believe
|
Sorry, some of my suggestions are causing rubocop issues, I don't have an editor with code open right now |
Co-authored-by: Andrejs <[email protected]>
Co-authored-by: Andrejs <[email protected]>
Co-authored-by: Andrejs <[email protected]>
b3d3fbc to
404d994
Compare
|
@andrcuns sorry, missed rubocop and one test |
|
Code Climate has analyzed commit 404d994 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (80% is the threshold). This pull request will bring the total coverage in the repository to 98.9% (0.0% change). View more on Code Climate. |
Adding
excludedandmodeoptions forParameterclass for Ruby binding