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.

Sign up for GitHub

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

Jump to bottom

[WIP] Allow to pass callable as argument of CodeDefinitionBuilder::setOptionValidator/setBodyValidator() #61

Closed
wants to merge 2 commits into from
Closed

[WIP] Allow to pass callable as argument of CodeDefinitionBuilder::setOptionValidator/setBodyValidator() #61

wants to merge 2 commits into from

Conversation

Kubo2
Copy link
Contributor

@Kubo2 Kubo2 commented Jul 28, 2015 •
edited
Loading

This is an implementation of the proposal #60.

  • implement ability to pass callables to setOptionValidator()/setBodyValidator()
  • refactor the code to match coding style
  • add tests

This change is Reviewable

@Kubo2 Kubo2 mentioned this pull request Jul 28, 2015
Open
@Kubo2 Kubo2 force-pushed the iss-60 branch 2 times, most recently from 438147d to 7c5b943 Compare July 28, 2015 09:42
@Kubo2 Kubo2 mentioned this pull request Jul 28, 2015
Merged
@Kubo2 Kubo2 changed the title Ability to pass a callable to CodeDefinitionBuilder::set*Validator() Allow to pass callable as argument of CodeDefinitionBuilder::setOptionValidator/setBodyValidator() Jul 29, 2015
@Kubo2
Copy link
Contributor Author

Kubo2 commented Jul 29, 2015

@jbowens Should I also fix the coding style for CallableValidatorAdapter.php or is it all well? Please could you review this?

@Kubo2 Kubo2 changed the title Allow to pass callable as argument of CodeDefinitionBuilder::setOptionValidator/setBodyValidator() [WIP] Allow to pass callable as argument of CodeDefinitionBuilder::setOptionValidator/setBodyValidator() Aug 14, 2015
@DaSourcerer
Copy link
Contributor

Shouldn't I split this into two separate commits, one with implementation, and one containing test case?

A bit late perhaps, but: PRs are ideally atomic. That is they contain the desired changes as well as additional tests to demonstrate the intended behaviour. Splitting code changes and tests into two PRs will also defeat the purpose of the CI.

@Kubo2 Kubo2 mentioned this pull request Feb 19, 2020
Merged
@Kubo2 Kubo2 closed this Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Kubo2 @DaSourcerer