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

Avoid initial new line when using custom group separator #29

Merged
merged 1 commit into from
Jun 9, 2022
Merged

Avoid initial new line when using custom group separator #29

merged 1 commit into from
Jun 9, 2022

Conversation

ArnaudBarre
Copy link
Contributor

@ArnaudBarre ArnaudBarre commented May 30, 2022

I think this screenshot will highlight what I feel like an inconsistency:

Screenshot 2022-05-30 at 15 15 22

Copy link
Collaborator

@fbartho fbartho left a comment

Choose a reason for hiding this comment

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

This feels like a good fix to me, I was noticing similar issues when I applied this to a large project. Thoughts @IanVS @blutorange?

@@ -70,8 +70,13 @@ export const getSortedNodesByImportOrder: GetSortedNodes = (nodes, options) => {
for (const group of importOrder) {
// If it's a custom separator, all we need to do is add a newline
if (isCustomGroupSeparator(group)) {
const lastNode = finalNodes[finalNodes.length - 1];
// Avoid empty new line if first group is empty
if (!lastNode) {
Copy link
Owner

Choose a reason for hiding this comment

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

This feels a little bit indirect. Could we check if finalNodes.length === 0? Would that accomplish the same thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be the same thing for this first checks, but then I would need to assert TS that the list is non empty for the next check (or add a useless runtime check)

@IanVS IanVS merged commit d7d2894 into IanVS:main Jun 9, 2022
@IanVS
Copy link
Owner

IanVS commented Jun 9, 2022

Thanks for the contribution, @ArnaudBarre!

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