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: sort dependencies #4

Closed
wants to merge 1 commit into from
Closed

feat: sort dependencies #4

wants to merge 1 commit into from

Conversation

will-stone
Copy link
Contributor

This PR contains:

  • bugfix
  • feature
  • refactor
  • tests
  • documentation
  • metadata

Breaking Changes?

  • yes
  • no

If users' dependencies are not alphabetically sorted, they will notice changes.

Please Describe Your Changes

Will alphabetically sort dependencies. Issue: #2

@shellscape
Copy link
Owner

Thanks for opening a PR 🍺

I'm still of the opinion that the solution in my comment on #2 would invariably be the most flexible solution for this. I don't believe the extra processing here is going to be necessary in most cases, and an option to specify additional property values to sort would cover the edge cases.

@will-stone
Copy link
Contributor Author

At the end of the day, it's your plugin so I'm happy to go with your judgement. However my counter-argument would be that we're already being very opinionated with ordering and so why stop at high-level keys? Especially as yarn and npm like to sort the keys in this way too. I'll leave this with you, feel free to tweak PR if you go down the opt-in route.

@shellscape
Copy link
Owner

Especially as yarn and npm like to sort the keys in this way too.

That's precisely why I'm hesitant. The package managers all handle this. I'd argue the edge case here is manually adding dependencies out of order to those arrays. It's not something that most users are going to do, so processing those arrays isn't something we should do on every run.

@will-stone
Copy link
Contributor Author

Right, gotcha. Yeah, maybe this isn't so useful. Feel free to close if this all seems like overkill.

@shellscape
Copy link
Owner

If you're up for it, I'd like to change this to provide that sortAlpha option. You've already laid most of the groundwork in your changes already.

@will-stone
Copy link
Contributor Author

Sure. Will do...

@will-stone
Copy link
Contributor Author

I had a go but couldn't figure out how to get the option to pass through to the plugin.

@shellscape
Copy link
Owner

OK let's keep this open. I'll follow up later today or tomorrow AM.

@shellscape
Copy link
Owner

OK so I'm not sure why this didn't automatically close, but your changes were merged in on 5df5f77. I chose to merge this as an intermediate step to the custom sort spec.

@shellscape shellscape closed this Oct 6, 2019
@shellscape
Copy link
Owner

So I've done the leg work for allowing custom sorting on the feat/sort-custom branch. Unfortunately, some shortsightedness by the Prettier team with regard to options has me pretty stuck. See prettier/prettier#6151

I need to give this some thought. We can't specify an object option, which would be ideal, so we'd have to use a really hacky end-around the problem: either have to have separate options for each sort method (e.g. sort-alpha or sortAlpha) and make users use a comma-separated string or some such, or maybe JSON. No good alternatives come to mind, so I'll have to think on it a bit.

@will-stone
Copy link
Contributor Author

Okay, thanks for looking into it.

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

Successfully merging this pull request may close these issues.

2 participants