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

fix: correctly handle empty params #36

Merged
merged 4 commits into from
Oct 11, 2021
Merged

fix: correctly handle empty params #36

merged 4 commits into from
Oct 11, 2021

Conversation

kubuzetto
Copy link
Contributor

No description provided.

@kubuzetto
fix: handle empty parameters properly
76ce7a5 
Signed-off-by: Devrim Şahin <devrim.sahin@picussecurity.com>
@kubuzetto
fix: revert to empty tags line
1023a4f 
Signed-off-by: Devrim Şahin <devrim.sahin@picussecurity.com>
@kubuzetto kubuzetto mentioned this pull request Oct 8, 2021
Closed
@kubuzetto kubuzetto changed the title Fix empty param bug fix: correctly handle empty params Oct 8, 2021
aeneasr
aeneasr requested changes Oct 9, 2021
View reviewed changes
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Could you please extract this logic into a separate func and add tests for it? That should resolve future regressions like the one introduced in the previous PR

@kubuzetto
Copy link
Contributor Author

Sure thing 👍 Where do your unit tests normally reside? I cannot locate them.

@kubuzetto
fix: separate exec calls as a builder
ed44280 
Signed-off-by: Devrim Şahin <devrim.sahin@picussecurity.com>
@aeneasr
Copy link
Member

aeneasr commented Oct 10, 2021

We don't have them yet :( That's probably the root cause of missing the previous bug :) You can just add a new cmd_test.go and add the test there :)

@kubuzetto
Copy link
Contributor Author

Ah ok 😄 Added a simple test for cmdBuilder

aeneasr
aeneasr approved these changes Oct 11, 2021
View reviewed changes
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Thank you!

@aeneasr aeneasr merged commit 8d89424 into ory:master Oct 11, 2021
@kubuzetto kubuzetto deleted the fix-empty-param-bug branch October 11, 2021 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aeneasr aeneasr aeneasr approved these changes

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
@kubuzetto @aeneasr