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

The data type for arguments in the extension should be a List<String> instead of a String #6

Open
bmuschko opened this issue Aug 5, 2021 · 3 comments · May be fixed by #17
Open

The data type for arguments in the extension should be a List<String> instead of a String #6

bmuschko opened this issue Aug 5, 2021 · 3 comments · May be fixed by #17
Labels
breaking change enhancement New feature or request

Comments

@bmuschko
Copy link

bmuschko commented Aug 5, 2021

Arguments are a list of Strings. From an end user's perspective it looks counter-intuitive to put together a single String for multiple arguments. See

. The handling on how those arguments are concatenated and used should be the responsibility of the plugin implementation.

snyk {
    arguments = "--dry-run --ignore-policy"
}

vs.

snyk {
    arguments = ["--dry-run", "--ignore-policy"]
}

Furthermore, the extension exposes an argument that is added to the list automatically: severity. That's not clear to the end user and could cause confusion when defined as part of arguments. Either keep the arguments as list and remove the field severity or introduce properties for each argument to be consistent.

@bmvermeer
Copy link
Collaborator

Interesting pov on the list although I do not fully agree.
The severity was there by design as it was in line with how the maven plugin at that time.

@bmvermeer bmvermeer added enhancement New feature or request breaking change labels Aug 5, 2021
@bmuschko
Copy link
Author

bmuschko commented Aug 5, 2021

Another point is that an end user might want to extend the list of arguments later e.g. from a plugin. How do you add a new argument with the String parameter? Even if you could the end user needs to ensure that the space is added for every argument which she can get wrong easily.

@trevjonez
Copy link

need to rework the extension to use the lazy gradle API's. so this should probably be a ListProperty

I think in most cases it probably won't matter that it would be a list but using a property should help ensure that people can do more advanced configuration without any extra burden on this project

@adam-dpg adam-dpg linked a pull request May 5, 2022 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants