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

feat: Add private accessors group to sort-member-config #41

Merged
merged 1 commit into from
Mar 24, 2021

Conversation

krshnpatel
Copy link
Contributor

Summary

While using lit-config extension in our project, I realized that having non-paired private accessors (i.e. private getter with no setter or vice versa) would intertwine the private accessors and private methods when sorting. So I added another group called "private-accessors" which sorts standalone getters first then sorts standalone setters second. See below for a better explanation.

This was the behaviour before this change:

// private accessor pairs group
get _method4() {}
set _method4(arg) {}

// private methods group
get _method1() {} // <-- private getter falls under "private-methods" group
_method2() {} // <-- private method
set _method3(arg) {} // <-- private setter falls under "private-methods" group
get _method5() {} // <-- private getter falls under "private-methods" group

This is the behaviour after this change:

// private accessor pairs group
get _method4() {}
set _method4(arg) {}

// *NEW* private accessors group
get _method1() {} // <-- getters sorted first
get _method5() {}
set _method3(arg) {} // <-- setters sorted second

// private methods group
_method2() {}

Copy link
Member

@dlockhart dlockhart left a comment

Choose a reason for hiding this comment

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

This seems good to me!

@dbatiste
Copy link
Collaborator

dbatiste commented Mar 24, 2021

@grant-cleary : I vaguely recall us looking at this a bit early on with the editor work. I don't remember whether this was the exact same use case we had or not. Not sure if you remember more...?

@grant-cleary
Copy link

@grant-cleary : I vaguely recall us looking at this a bit early on with the editor work. I don't remember whether this was the exact same use case we had or not. Not sure if you remember more...?

Yup, this was the same use case.

@krshnpatel
Copy link
Contributor Author

@dbatiste @grant-cleary : Is there another issue that we need to deal with before I merge this in? Or this is good to go?

@dbatiste
Copy link
Collaborator

dbatiste commented Mar 24, 2021

I think this looks fine. I seem to remember Grant and I being somewhat dissatisfied with our ordering when we were looking at this previously, but couldn't remember exactly what it was, and whether Grant remembered.

@grant-cleary
Copy link

grant-cleary commented Mar 24, 2021

I think this looks fine. I seem to remember Grant and I being somewhat dissatisfied with our ordering when we were looking at this previously, but could remember exactly what it was, and whether Grant remembered.

Yeah, I don't 100% remember why we decided not to fix this. We might not have cared enough? 😕

Copy link
Contributor

@svanherk svanherk left a comment

Choose a reason for hiding this comment

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

🎉 Seems like a great idea!

@krshnpatel krshnpatel merged commit 0867c06 into master Mar 24, 2021
@krshnpatel krshnpatel deleted the add-private-accessors branch March 24, 2021 18:34
@ghost
Copy link

ghost commented Mar 24, 2021

🎉 This PR is included in version 0.14.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants