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 add helping messages #4472

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

Conversation

AlexV525
Copy link

Resolves #4470


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.

@AlexV525 AlexV525 marked this pull request as ready for review December 13, 2024 13:55
Copy link
Contributor

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

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

Personally I'd prefer just fixing the help-text. I think it is very hard to make regex updates inside yaml without breaking it.

lib/src/command/add.dart Outdated Show resolved Hide resolved
lib/src/command/add.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

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

Also note, that if you leave the quotes yaml should accept the the examples as given...

@AlexV525
Copy link
Author

Personally I'd prefer just fixing the help-text. I think it is very hard to make regex updates inside yaml without breaking it.

Let's add more test cases when we meet them. :) There is nothing broken with the given pattern examples.

Also note, that if you leave the quotes yaml should accept the the examples as given...

Are you saying that 'foo:{"path":"../foo"}' should work? If yes then it still does after the change.

@sigurdm
Copy link
Contributor

sigurdm commented Dec 13, 2024

Are you saying that 'foo:{"path":"../foo"}' should work? If yes then it still does after the change.

I'm saying 'foo:{"hosted":"https://pub.dev"}' should work. And all other yaml that I cannot imagine, but I don't want to process it up-front.

@AlexV525
Copy link
Author

I'm saying 'foo:{"hosted":"https://pub.dev"}' should work. And all other yaml that I cannot imagine, but I don't want to process it up-front.

Sure. All of the previous examples still work.

@sigurdm
Copy link
Contributor

sigurdm commented Dec 13, 2024

Does it not insert a space into the host url?

@sigurdm
Copy link
Contributor

sigurdm commented Dec 13, 2024

Does it not insert a space into the host url?

void main() {
  print('foo:{"hosted":"https://pub.dev"}'.replaceAllMapped( RegExp(r'([,:])(\S)'),
          (match) => '${match.group(1)} ${match.group(2)}'));
}

Prints

foo: {"hosted": "https: //pub.dev"}

@AlexV525 AlexV525 force-pushed the fix/add-syntax-parse branch from 86cd753 to 61fef76 Compare December 13, 2024 15:36
@AlexV525 AlexV525 changed the title Fix add argument syntax parsing Update add helping messages Dec 13, 2024
@AlexV525
Copy link
Author

The request has been updated to change messages only.

@AlexV525 AlexV525 force-pushed the fix/add-syntax-parse branch from 61fef76 to 68f80bb Compare December 13, 2024 15:38
@sigurdm
Copy link
Contributor

sigurdm commented Dec 16, 2024

FYI: If you delete the golden file, and run the test it will be recreated correctly

@AlexV525
Copy link
Author

AlexV525 commented Dec 16, 2024

FYI: If you delete the golden file, and run the test it will be recreated correctly

Yeah sorry about repeatedly commits. I thought it was just an easy update. 🥲

EDIT: FYI The generated header is different on Windows: # GENERATED BY: test\help_test.dart

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.

On powershell pub add given examples fails when yaml contain double quotes
2 participants