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

docs: revise Cucumber format option #3024

Closed
wants to merge 4 commits into from
Closed

docs: revise Cucumber format option #3024

wants to merge 4 commits into from

Conversation

tianfeng92
Copy link
Contributor

@tianfeng92 tianfeng92 commented Nov 25, 2024

Description

Update docs for Cucumber format config after cucumber deprecated the no quote option.

Motivation and Context

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation fix (typos, incorrect content, missing content, etc.)

Copy link

Deploy preview ready for 3024!
https://docs.dev.saucelabs.net/pr-preview/pr-3024

Copy link

Deploy preview ready for 3024!
https://docs.dev.saucelabs.net/pr-preview/pr-3024

1 similar comment
Copy link

Deploy preview ready for 3024!
https://docs.dev.saucelabs.net/pr-preview/pr-3024

@tianfeng92 tianfeng92 marked this pull request as ready for review November 25, 2024 23:12
@tianfeng92 tianfeng92 requested a review from a team as a code owner November 25, 2024 23:12
Comment on lines 1265 to 1271
:::warning
Cucumber deprecated the previous `format` configuration without quotes in version **9.6.0** and will remove it in **11.0.0** or later.
If you're still using a `format` setting without quotes, migrate to the latest pattern.
For more details, see the [Cucumber.js Deprecations documentation](https://github.com/cucumber/cucumber-js/blob/main/docs/deprecations.md#ambiguous-colons-in-formats).

The `file://my_formatter/implementation:output_file` is an example of ambiguous colon usage and is not allowed in Sauce Labs now. You should migrate to the quoted pattern.
:::
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this level of detail makes things a little more confusing. Also, I'm not entirely sure what's written here is correct. The deprecation docs says:

Cucumber tries to detect and handle things like Windows drives and file:// URLs on a best-effort basis, but this logic is being removed

And that suggests to me its not unquoted values that will be deprecated, its the logic to handle ambiguous colons.

Plus if the runner handles all the quoting anyways, does the user need to know all this?

Copy link
Contributor Author

@tianfeng92 tianfeng92 Nov 26, 2024

Choose a reason for hiding this comment

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

hmm.. I agree adding this specific case here is confusing. But I have no idea whether I should point out the file://my_formatter/implementation:output_file can't be used anymore if some users did set it before. At least this is a breaking change. Besides, I don't want to put it in the YAML example to explicitly let them know there is a "file://" option, which could be potential issue.

How about we just mention Cucumber deprecated the previous format and leave a link to their docs? And use the latest pattern in our YAML example.

Copy link

Deploy preview ready for 3024!
https://docs.dev.saucelabs.net/pr-preview/pr-3024

Copy link
Collaborator

@alexplischke alexplischke left a comment

Choose a reason for hiding this comment

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

@tianfeng92 Do we actually need to update the docs on this?
Having to explicitly quote all this is pretty annoying as a user. And you already added the convenient logic in the runner that auto quotes for the user. So technically speaking, as long as our own users are using the upcoming runner, they won't really have to do any changes in our configuration going forward.

@tianfeng92 tianfeng92 closed this Nov 26, 2024
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.

3 participants