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 change_builder_dart.dart #55383

Closed
wants to merge 2 commits into from
Closed

Conversation

Devenik7
Copy link
Contributor

@Devenik7 Devenik7 commented Apr 5, 2024

Added support for named parameters while generating super method invocation

  • Thanks for your contribution! Please replace this text with a description of what this PR is changing or adding and why, list any relevant issues, and review the contribution guidelines below.

  • [YES] I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:
  • See our contributor guide for general expectations for PRs.
  • Larger or significant changes should be discussed in an issue before creating a PR.
  • Contributions to our repos should follow the Dart style guide and use dart format.

Note that this repository uses Gerrit for code reviews. Your pull request will be automatically converted into a Gerrit CL and a link to the CL written into this PR. The review will happen on Gerrit but you can also push additional commits to this PR to update the code review.

Added support for named parameters while generation super method invocation
Copy link

Thank you for your contribution! This project uses Gerrit for code reviews. Your pull request has automatically been converted into a code review at:

https://dart-review.googlesource.com/c/sdk/+/361282

Please wait for a developer to review your code review at the above link; you can speed up the review if you sign into Gerrit and manually add a reviewer that has recently worked on the relevant code. See CONTRIBUTING.md to learn how to upload changes to Gerrit directly.

Additional commits pushed to this PR will update both the PR and the corresponding Gerrit CL. After the review is complete on the CL, your reviewer will merge the CL (automatically closing this PR).

@srawlins
Copy link
Member

srawlins commented Apr 5, 2024

Thanks for the PR! Can you add a test or a few tests. Likely with the same tests where @scheglov initially wrote the @override completion feature, in https://dart-review.googlesource.com/c/sdk/+/356640.

@Devenik7
Copy link
Contributor Author

Devenik7 commented Apr 6, 2024

Got it.

Let me go through that original change and figure stuff out. First time contributing to Open Source.

@Devenik7
Copy link
Contributor Author

Devenik7 commented Apr 9, 2024

Thanks for the PR! Can you add a test or a few tests. Likely with the same tests where @scheglov initially wrote the @override completion feature, in https://dart-review.googlesource.com/c/sdk/+/356640.

Hi @srawlins. I have added 2 tests, one for a method with only positional parameters and one for a method with both positional and named parameters.

Could you check and suggest if anything else is needed.

Copy link

https://dart-review.googlesource.com/c/sdk/+/361282 has been updated with the latest commits from this pull request.

1 similar comment
Copy link

https://dart-review.googlesource.com/c/sdk/+/361282 has been updated with the latest commits from this pull request.

@srawlins
Copy link
Member

The new tests seem to fail. See https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8751123390552515201/+/u/test_results/new_test_failures__logs_

Also, we sort our files alphabetically (just the analyzer team), which can be done in an IDE with a "sort members" command. See the same log for "class_body_test.dart" being an "Unsorted file".

@Devenik7
Copy link
Contributor Author

Let me check that

@devoncarew devoncarew requested review from srawlins and removed request for srawlins April 15, 2024 15:23
Copy link

https://dart-review.googlesource.com/c/sdk/+/361282 has been updated with the latest commits from this pull request.

@srawlins
Copy link
Member

I'm still seeing a failure at https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8750577436591192721/+/u/test_results/new_test_failures__logs_, see "test_class_method_without_namedParameters".

@Devenik7
Copy link
Contributor Author

Hi @srawlins. Have been busy with my day job. I have checked the errors and understood the changes needed.

Will get back (probably over the weekend) with the changes.

@mraleph
Copy link
Member

mraleph commented Apr 30, 2024

I assume this is stale. Please reopen if not.

@mraleph mraleph closed this Apr 30, 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.

None yet

3 participants