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

feat: allow 'integer' as an action param type #67

Merged

Conversation

tonyandrewmeyer
Copy link
Collaborator

This extends the consistency checker for action parameters to allow the JSON Schema integer type (aligning with charmcraft and juju), which maps to a Python int.

The tests are extended to include this as well as simple consistency check tests for the other types.

Fixes #66

@tonyandrewmeyer
Copy link
Collaborator Author

The test failure seems unrelated to this change, but if that's not the case, please let me know and I'll dig into it.

@PietroPasotti
Copy link
Collaborator

The test failure seems unrelated to this change, but if that's not the case, please let me know and I'll dig into it.

yup, they're fixed in #65

In light of your remarks in the spec on adding action testing in harness, what do you think about adding a .has_failed() or is_failure method to ActionOutput, to put some sugar on top of .failure is not None?

@PietroPasotti
Copy link
Collaborator

can you rebase now that #65 has merged? the errors should be gone

@PietroPasotti
Copy link
Collaborator

thanks a lot!

@PietroPasotti PietroPasotti merged commit ca13b72 into canonical:main Oct 19, 2023
2 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the allow-integer-action-params branch October 19, 2023 21:19
@tonyandrewmeyer
Copy link
Collaborator Author

In light of your remarks in the spec on adding action testing in harness, what do you think about adding a .has_failed() or is_failure method to ActionOutput, to put some sugar on top of .failure is not None?

Just so it's clear: the comments in the spec were not meant as any criticism of scenario - I think it's great, and the approach I took was heavily inspired by scenario.

I do think it's nicer to have some sugar on top of the failure message itself, since the message can be (and defaults to) the empty string, so assert not out.failure will sometimes be ok but sometimes not. I feel a property is nicer than a method since it's really just sugar over a simple is None.

Before we settled on the exception approach, @benhoyt and I favoured out.success, because it seems like most of the time that's what you're expecting (success plus some action thing done), so assert out.success is short and clear. But assert not out.is_failure is pretty good too.

@PietroPasotti
Copy link
Collaborator

Thanks!
I'll play around out.success and see how it feels :)

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.

"integer" is not accepted as an action param type
2 participants