Skip to content

Updated addVariable() and updateVariable() #251

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

Merged
merged 3 commits into from
Sep 29, 2017

Conversation

c33s
Copy link
Contributor

@c33s c33s commented Sep 19, 2017

updated addVariable() and updateVariable() to match current gitlab api (added $protected $environment_scope`)

@c33s
Copy link
Contributor Author

c33s commented Sep 19, 2017

an idea also would be to do some input validation with webmozart/assert

Assert::nullOrBoolean($protected)
Assert::nullOrString($environment_scope)

@c33s c33s changed the title updated addVariable and updateVariable WIP: updated addVariable and updateVariable Sep 19, 2017
…tests for `addVariable()`, added tests for `updateVariable()`
@c33s c33s changed the title WIP: updated addVariable and updateVariable Updated addVariable() and updateVariable() Sep 20, 2017
@c33s
Copy link
Contributor Author

c33s commented Sep 20, 2017

@m1guelpf what about the suggestion to add assert? this would add a new dependency to the code but will improve the error detection. but it is a very stable dependency (26M downloads ). it is from @webmozart inpired by beberlei/assert.

@fbourigault
Copy link
Contributor

We started using OptionsResolver to validate filters/payloads.

@c33s
Copy link
Contributor Author

c33s commented Sep 29, 2017

anything missing from my side that you can merge this PR?

Copy link
Member

@m1guelpf m1guelpf left a comment

Choose a reason for hiding this comment

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

Again, 👍
@fbourigault Your opinion?

@fbourigault
Copy link
Contributor

Thank you!

@fbourigault fbourigault merged commit e37b78f into GitLabPHP:master Sep 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants