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

Fixed Action building logic in JsonObjectFromPolicyTransformer #4842

Conversation

milux
Copy link

@milux milux commented Feb 25, 2025

What this PR changes/adds

Fixes #4838.

Why it does that

The action is wrapped into another action under certain circumstances, which, according to our understanding of the ODRL IM, seems to be an error.

Linked Issue(s)

Closes #4838

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

We are always happy to welcome new contributors ❤️ To make things easier for everyone, please make sure to follow our contributors manual, check if you have already signed the ECA, and relate this pull request to an existing issue or discussion.

@jimmarino
Copy link
Contributor

Please see my comments in the issue on how this should be fixed (i.e. it must correspond to the DSP schema),

@paullatzelsperger
Copy link
Member

paullatzelsperger commented Feb 25, 2025

@milux please also respect our process. issues must be triaged before working on them, and you will need a sponsor.
in addition, there are no tests on this, which is a definite death sentence for any PR.

for reference: https://eclipse-edc.github.io/documentation/for-contributors/guidelines/pr-etiquette/

@milux milux closed this Feb 25, 2025
@milux
Copy link
Author

milux commented Feb 25, 2025

@milux please also respect our process. issues must be triaged before working on them, and you will need a sponsor. in addition, there are no tests on this, which is a definite death sentence for any PR.

for reference: https://eclipse-edc.github.io/documentation/for-contributors/guidelines/pr-etiquette/

@paullatzelsperger I would have been happy to adapting any tests covering that code. Since I essentially only removed 2 lines, I can hardly see how this would negatively affect coverage, though.

Since the corresponding issue was already closed, that doesn't matter anymore.

@paullatzelsperger
Copy link
Member

I would have been happy to adapting any tests covering that code. Since I essentially only removed 2 lines, I can hardly see how this would negatively affect coverage, though.

It may be a moot point, but still: a change like this always warrants a test. If you can't see that, then that is a problem.

@milux
Copy link
Author

milux commented Feb 25, 2025

It may be a moot point, but still: a change like this always warrants a test. If you can't see that, then that is a problem.

I can perfectly understand that nobody wants contribs that reduce coverage or break existing tests, and my PR did the latter, so my bad.
On the other hand, demanding voluntary contributors to write tests for code that doesn't break any tests because there was no testing coverage to begin with, would be a moot point indeed. I personally wouldn't ask anybody to meet quality standards that I can't uphold myself, but that's your show, so you can demand whatever seems appropriate to you.

@paullatzelsperger
Copy link
Member

paullatzelsperger commented Feb 25, 2025

Writing tests is simply good practice and not up for debate. While our code base might be lacking in that regard (and maybe others) on occasion, it cannot serve as reasoning to not strive to do better. Quite the opposite.

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.

ODRL policy output error
3 participants