Skip to content

feat: add multiple getters mode in generate_getter #13365

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

Merged
merged 1 commit into from
Oct 20, 2022

Conversation

feniljain
Copy link
Contributor

This commit adds two modes to generate_getter action.
First, the plain old working on single fields.
Second, working on a selected range of fields.

Should partially solve #13246
If this gets approved will create a separate PR for setters version of the same

Points to help in review:

  • generate_getter_from_record_info contains code which is mostly taken from assist before refactor
  • Same goes for parse_record_fields
  • There are changes in other assists, as one of the methods in utils named find_struct_impl is changed, before it used to accept a single fn_name, now it takes a list of function names to check against. All old impls are updated to create a small list to pass their single element.

Assumptions:

  • If any of the fields have an implementation, the action will quit.

@feniljain feniljain marked this pull request as ready for review October 8, 2022 11:26
@bors
Copy link
Contributor

bors commented Oct 17, 2022

☔ The latest upstream changes (presumably #13399) made this pull request unmergeable. Please resolve the merge conflicts.

@feniljain feniljain force-pushed the master branch 2 times, most recently from cbbe4da to 9859ace Compare October 19, 2022 07:38
@feniljain
Copy link
Contributor Author

Hey @Veykril , would love to get a review on this!

@feniljain feniljain force-pushed the master branch 2 times, most recently from 00acca4 to 16d888e Compare October 20, 2022 11:16
This commit adds two modes to generate_getter action.
First, the plain old working on single fields.
Second, working on a selected range of fields.
@feniljain
Copy link
Contributor Author

Thanks for in depth and amazing review, have resolved all of them now!

@feniljain feniljain requested a review from Veykril October 20, 2022 11:37
@Veykril
Copy link
Member

Veykril commented Oct 20, 2022

Thanks!
Small note, try to not force push when you address reviews, unless you need to due to a rebase or similar. A PR can have multiple commits if need be. (This makes it easier to see what you have changed in regards to a review).

@bors r+

@bors
Copy link
Contributor

bors commented Oct 20, 2022

📌 Commit 5bff6c5 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 20, 2022

⌛ Testing commit 5bff6c5 with merge f3cce5f...

@feniljain
Copy link
Contributor Author

Small note, try to not force push when you address reviews, unless you need to due to a rebase or similar. A PR can have multiple commits if need be. (This makes it easier to see what you have changed in regards to a review).

Oh will keep that in mind, thnx for the wisdom 🙇

@bors
Copy link
Contributor

bors commented Oct 20, 2022

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing f3cce5f to master...

1 similar comment
@bors
Copy link
Contributor

bors commented Oct 20, 2022

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing f3cce5f to master...

@bors
Copy link
Contributor

bors commented Oct 20, 2022

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

@bors bors merged commit f3cce5f into rust-lang:master Oct 20, 2022
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