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

infer variable name improvements #72

Merged
merged 4 commits into from
Sep 16, 2022

Conversation

zardoy
Copy link
Contributor

@zardoy zardoy commented Sep 15, 2022

Sep-15-2022 05-01-17

  • also needs fix for something.messages.forof

@zardoy
Copy link
Contributor Author

zardoy commented Sep 15, 2022

I thing I'm going to revert last commit as there is no need to display not clean variant.
Though it was interesting to explore. I thing it might be useful in future to suggest multiple variants for cases like: users[0].const -> firstUser|user

@ipatalas
Copy link
Owner

Wow, that's super cool! I love the idea!

package.json Outdated Show resolved Hide resolved
src/templates/varTemplates.ts Outdated Show resolved Hide resolved
auto-merge was automatically disabled September 16, 2022 08:31

Head branch was pushed to by a user without write access

@zardoy
Copy link
Contributor Author

zardoy commented Sep 16, 2022

Though I still didn't figure out how to fix something.messages.forof template

@ipatalas ipatalas merged commit 1c7f8a2 into ipatalas:develop Sep 16, 2022
@ipatalas
Copy link
Owner

Though I still didn't figure out how to fix something.messages.forof template

What do you mean? How is it broken?
image
Seems to work unless you mean to infer message from that - if so then it's super easy, I'll handle that.

@ipatalas
Copy link
Owner

Two lines of code and one minute later:
image

@ipatalas
Copy link
Owner

Also made another small improvement:
image

@zardoy
Copy link
Contributor Author

zardoy commented Sep 17, 2022

From my POV these cases are pretty common and we should support them:

for (let command of MenuRegistry.getCommands()) {
}

Shouldn't be hard since we have both extraction functions in place, try to do this soon or you can handle them

@ipatalas
Copy link
Owner

Correct, that's a very common case. I'm on my phone now but i already know it's another 2 lines of code so i will implement that if i have 15 minutes this weekend 😉

@zardoy
Copy link
Contributor Author

zardoy commented Sep 17, 2022

Also what do you think of this pattern? Do you think its common and we can handle it?

for (let indexToPatch of indexesToPatch) {
}

index -> indexes
Edit: I think it can be related to #72 (comment) (not about additional, suggestions)

ipatalas added a commit that referenced this pull request Sep 18, 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.

None yet

2 participants