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

update config to support default definition function #2384

Merged

Conversation

notaphplover
Copy link
Contributor

@notaphplover notaphplover commented Mar 20, 2024

🤔 What's changed?

This PR aims to allow default profile to be a function that dynamically generates cucumber profiles

⚡️ What's your motivation?

Closes #2009.

🏷️ What kind of change is this?

  • ⚡ New feature (non-breaking change which adds new behaviour)

♻️ Anything particular you want feedback on?

📋 Checklist:

  • I agree to respect and uphold the Cucumber Community Code of Conduct
  • I've changed the behaviour of the code
    • I have added/updated tests to cover my changes.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

@coveralls
Copy link

coveralls commented Mar 20, 2024

Coverage Status

coverage: 98.397% (+0.005%) from 98.392%
when pulling 5699c69 on notaphplover:feat/add-default-config-function
into a02d90e on cucumber:main.

Copy link
Contributor

@davidjgoss davidjgoss left a comment

Choose a reason for hiding this comment

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

Thanks for raising @notaphplover, this is looking good.

A few initial thoughts from me, if that's okay:

  • It's neat that we allow static profile definitions to be done on other exports when doing the function, but that feels like a bit of a confusing API to me - I think it might be better to keep it simple, so you either a) provide static profiles with exports, or b) a single default export function to do it dynamically, and not a mixture of the two. What do you think?
  • I also think we should avoid adding the default: {} in the new code - if the dynamically resolved config has no default key, I still think we want to have that logging happen below

Other than that, I think the next steps here would be:

Let me know if you need a steer on anything!

@notaphplover
Copy link
Contributor Author

notaphplover commented Apr 5, 2024

Thanks for raising @notaphplover, this is looking good.

You're very welcome, it's my pleasure 😃.

A few initial thoughts from me, if that's okay:

  • It's neat that we allow static profile definitions to be done on other exports when doing the function, but that feels like a bit of a confusing API to me - I think it might be better to keep it simple, so you either a) provide static profiles with exports, or b) a single default export function to do it dynamically, and not a mixture of the two. What do you think?

I totally agree with you.

  • I also think we should avoid adding the default: {} in the new code - if the dynamically resolved config has no default key, I still think we want to have that logging happen below

I see, but then in some cases there will be no default profile. You know way better than me the code flows so I assume that's ok. Sure, I'll add the logging sentence.

Other than that, I think the next steps here would be:

Sure, I'll go for it.

Let me know if you need a steer on anything!

Thank you so much! Sure, I'll do it. I'll try to have some time to continue on this on the weekend.

@davidjgoss
Copy link
Contributor

I see, but then in some cases there will be no default profile. You know way better than me the code flows so I assume that's ok. Sure, I'll add the logging sentence.

Ah you're right, looking at the code again I see that the else block with the logging would not be reached in the case of a config-providing function. It would be good to make that happen either way by shifting the code around a bit.

@notaphplover notaphplover marked this pull request as ready for review April 12, 2024 11:55
Copy link
Contributor

@davidjgoss davidjgoss left a comment

Choose a reason for hiding this comment

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

Thanks very much @notaphplover!

@davidjgoss davidjgoss merged commit 66d0b1b into cucumber:main Apr 21, 2024
8 checks passed
@davidjgoss
Copy link
Contributor

Released in https://github.com/cucumber/cucumber-js/releases/tag/v10.5.0

@notaphplover notaphplover deleted the feat/add-default-config-function branch April 21, 2024 15:08
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.

Allow dynamically defining profiles on esm configs
3 participants