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

Add snake_case to params normalization #3884

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ahvigil
Copy link

@ahvigil ahvigil commented Apr 19, 2023

fixes #3878. There are two changes here:

  • snake_case pipeline param names are now handled in a way that is consistent with what nextflow does for kebab-case and camelCase params, which are already interchangeable on the command line. That means --long-name, --longName, and --long_name are parsed as the same param.
  • change the way param names get normalized so that instead of storing duplicate entries in the ParamsMap for each parameter representation, we normalize before accessing the underlying map. With the previous implementation, ParamsMap('foo-bar':1) produces a ParamsMap of size 2, which seems kind of confusing.

@ahvigil ahvigil force-pushed the master branch 2 times, most recently from 9397ed1 to 390effc Compare April 19, 2023 17:34
Signed-off-by: Arthur Vigil <arthur.vigil@invitae.com>
@@ -133,7 +134,7 @@ class ScriptBindingTest extends Specification {

then:
map['field1'] == 1
map['field-1'] == null
Copy link
Author

Choose a reason for hiding this comment

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

this test case changes because camel case is a bit ambiguous with numbers- field-1 normalizes to field1 but field1 wasn't converting via camelCaseToHyphen into field-1.

@@ -184,7 +185,7 @@ class ScriptBindingTest extends Specification {
when:
def params = new ScriptBinding.ParamsMap('foo-bar':1)
then:
params.size() == 2
params.size() == 1
Copy link
Author

Choose a reason for hiding this comment

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

ParamsMap.size now more accurately reflects the number of params it contains.

@ahvigil ahvigil marked this pull request as ready for review April 19, 2023 17:47
@ahvigil
Copy link
Author

ahvigil commented Apr 26, 2023

CLI is being rewritten in #3600 so this code will soon be deprecated, closing

@ahvigil ahvigil closed this Apr 26, 2023
@bentsherman
Copy link
Member

Since the params normalization code is separate from the CLI, it should not be affected by the new CLI, so let's keep this open.

However I would prefer to merge this fix #4188 first.

@bentsherman bentsherman reopened this Sep 18, 2023
@bentsherman bentsherman changed the title normalize CLI pipeline params Add snake_case to params normalization Sep 18, 2023
Copy link

netlify bot commented Dec 15, 2023

Deploy Preview for nextflow-docs-staging ready!

Name Link
🔨 Latest commit 753ad9d
🔍 Latest deploy log https://app.netlify.com/sites/nextflow-docs-staging/deploys/65ae96d284b29f0008ec7ee5
😎 Deploy Preview https://deploy-preview-3884--nextflow-docs-staging.netlify.app/cli
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Signed-off-by: Arthur Vigil <arthur.vigil@invitae.com>
ahvigil and others added 2 commits December 19, 2023 12:48
Signed-off-by: Arthur Vigil <arthur.vigil@invitae.com>
@ahvigil
Copy link
Author

ahvigil commented Dec 19, 2023

updated with merge from master to incorporate changes from #4188 with necessary updates, and made a small update to mention in the docs

@ahvigil ahvigil requested a review from a team as a code owner April 3, 2024 14:39
@Nish9
Copy link

Nish9 commented Aug 13, 2024

Looks like this is not merged. Any idea when this feature will be available?

@bentsherman
Copy link
Member

Unfortunately I don't think this will be merged soon because the params are being overhauled (#4669) and this change would likely need to be incorporated into that. Feel free to comment there to share your thoughts.

I have been thinking about snake case though, and I think, if we have a params schema then we can intelligently convert kebab-case CLI params to either snake-case or camel-case depending on whether that param is defined in the schema.

Also there is no need to keep the kebab-case params in the params map for the script since you wouldn't access those anyway (see #4702)

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.

add logic for normalizing snake_case pipeline params
3 participants