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

Fix puretext and add option FavoriteQuoteChar (for string snippet) #474

Closed
wants to merge 6 commits into from

Conversation

RealByron
Copy link

@RealByron RealByron commented Nov 5, 2023

better for this complain
#470

and allow cucumber parameter in pureTextStep

dependabot bot and others added 6 commits November 4, 2023 18:17
Bumps [debug](https://github.com/debug-js/debug) to 4.3.4 and updates ancestor dependency [mocha](https://github.com/mochajs/mocha). These dependencies need to be updated together.


Updates `debug` from 2.6.8 to 4.3.4
- [Release notes](https://github.com/debug-js/debug/releases)
- [Commits](debug-js/debug@2.6.8...4.3.4)

Updates `mocha` from 3.5.3 to 10.2.0
- [Release notes](https://github.com/mochajs/mocha/releases)
- [Changelog](https://github.com/mochajs/mocha/blob/master/CHANGELOG.md)
- [Commits](mochajs/mocha@v3.5.3...v10.2.0)

---
updated-dependencies:
- dependency-name: debug
  dependency-type: indirect
- dependency-name: mocha
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <support@github.com>
also fix when smartSnippets is set to false
Copy link

@RobbieDixonBr-dge RobbieDixonBr-dge left a comment

Choose a reason for hiding this comment

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

The code looks great. I have some doubts around needing to add a new cucumberautocomplete setting to provide this functionality, rather than having it as something that runs in the background.

@@ -359,7 +358,12 @@ export default class StepsHandler {
//We can replace some outputs, ex. strings in brackets to make insert strings more neat
res = res.replace(/\"\[\^\"\]\+\"/g, '""');
Copy link

@RobbieDixonBr-dge RobbieDixonBr-dge Nov 6, 2023

Choose a reason for hiding this comment

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

In other areas of the code, we are returning double quotes without some conditional logic to return double or single quotes.

Do we want to introduce a new cucumberautocomplete setting to give the user the option of double/single quotes or do we just want to return double quotes every time with no additional cucumberautocomplete setting?

Copy link
Author

Choose a reason for hiding this comment

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

It's fine to not have this option. My concerne was to not change current habits of users if they want to

Copy link

@RobbieDixonBr-dge RobbieDixonBr-dge Nov 6, 2023

Choose a reason for hiding this comment

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

Is there some use case for the following scenarios:

image

This always seemed like a bug to me rather than desirable functionality.

I guess it lets the user know that they can use double or single quotes but maybe this could be highlighted in the documentation rather than every time a string is autocompleted.

Happy to discuss this further as, like you say, we don't want to change current user habits if they are actually benefitting from the current implementation.

Copy link

@vitalets vitalets Nov 8, 2023

Choose a reason for hiding this comment

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

This always seemed like a bug to me rather than desirable functionality.

Same for me.
Can we just generate When I click the ""?

@@ -130,6 +130,8 @@ By default, all the `' ' "` symbols will be used do define start and the end of
**`cucumberautocomplete.pureTextSteps`** - Some frameworks using gherkin steps as a text with just support for the cucumber expression instead of RegExp. This differs from the default extension behaviour, example:
`When('I give 5$ and * items')` step would be handled as `/I give 5$ and * items/` RegExp without this option enabled and as `/^I give 5\$ and \* items$/` RegExp with it (`^` and `$` symbols were added to the reg ex and also all the special regex symbols were handled as regular text symbols).

**`favoriteQuoteChar`** - could be `'` or `"`. Quote char used during auto-completion of `{string}` parameters. It provides a better typing experience with smartSnippets.
Copy link

@RobbieDixonBr-dge RobbieDixonBr-dge Nov 6, 2023

Choose a reason for hiding this comment

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

Does favoriteQuoteChar have a dependency on smartSnippets? Or does it just complement smartSnippets?

Copy link
Author

Choose a reason for hiding this comment

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

It work without smartSnippets set to true
if smartSnippets is set to false it produces "[^"]*"
if smartSnippets is set to true it produces "${1:}"



const favQuote = this.settings.cucumberautocomplete.favoriteQuoteChar;
if (favQuote){

Choose a reason for hiding this comment

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

If you don't provide the favoriteQuoteChar setting, is it set to "\"" by default, or is it set to null?

Isn't this functionality the type that the entire user base could benefit from? Rather than users having to update their cucumberautocomplete settings before they can benefit from it?

Copy link
Author

Choose a reason for hiding this comment

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

it is set to null.
and produces usual output : ("|')${1:}\1

Copy link
Author

Choose a reason for hiding this comment

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

oh sorry by default favoriteQuoteChar is set to """
I think it's time to suppress this option so

Choose a reason for hiding this comment

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

If we remove this setting then this if statement is redundant, yes!

@RobbieDixonBr-dge
Copy link

@RealByron do you have the capacity to make the changes above?

If not, I can maybe extend your PR but it will need to be re-reviewed.

@RealByron RealByron closed this Dec 2, 2023
@RealByron RealByron deleted the fix_puretext branch December 2, 2023 13:56
@RobbieDixonBr-dge
Copy link

:(

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.

None yet

3 participants