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

Add sort-direction option #192

Merged
merged 2 commits into from Apr 28, 2019

Conversation

TimonVS
Copy link
Member

@TimonVS TimonVS commented Apr 27, 2019

This PR adds the sort-direction option. Valid values are ascending or descending. I do notice that the list of configuration options is getting quite long, we may want to split up more advanced configuration options like this one into another document in the near future.

Fixes: #188

@TimonVS TimonVS requested a review from toolmantim April 27, 2019 14:52
@TimonVS
Copy link
Member Author

TimonVS commented Apr 27, 2019

I can't select people who aren't a member of the repo as a reviewer 🤔. But I'd appreciate @Casz review as well 🙇‍♂️.

@jetersen
Copy link
Member

I'd be happy to be considered a member of the repo 👍 💪

@@ -27,6 +33,7 @@ module.exports = app => {
context,
replacers: config.replacers
})
config['sort-direction'] = validateSortDirection(config['sort-direction'])
Copy link
Member Author

Choose a reason for hiding this comment

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

It's probably better to solve this with a Joi schema?

Copy link
Member

Choose a reason for hiding this comment

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

I'll see if I can get around to it today 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

No need to hurry! If you don't get around to it we can simply create an issue to improve this later :)

Copy link
Member

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

Looks mighty good to me 👍
GraphQL 🙇‍♂️

Copy link
Collaborator

@toolmantim toolmantim left a comment

Choose a reason for hiding this comment

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

Looks ace! Just a small nitpick/question about the console logs

lib/sort-pull-requests.js Outdated Show resolved Hide resolved
@jetersen
Copy link
Member

Should be easy for me to add into schema so perhaps my PR supersedes his? 😆

@toolmantim toolmantim merged commit 2db8af2 into release-drafter:master Apr 28, 2019
@TimonVS TimonVS deleted the feature/sorting-direction branch April 29, 2019 10:02
@toolmantim
Copy link
Collaborator

So, after doing a quick test, I'm thinking we should reverse the default sort order @TimonVS, and make it ascending. What do you reckon from your testing?

@TimonVS
Copy link
Member Author

TimonVS commented Apr 29, 2019

Your call, ascending kind of was the default before so I'm fine with changing it to ascending. Personally I think I prefer descending but I don't have a strong opinion on it :)

@toolmantim
Copy link
Collaborator

Cool. I remember why I thought that maybe ascending was better again — I found while editing the latest release notes for Release Drafter, if you have PRs that build upon one another, they don't make much sense in reverse chronological order and takes more mental gymnastics.

But TBH, I'm not really fussed enough to change it either. Let's wait for feedback and see.

@jetersen
Copy link
Member

My 2 cents is I found ascending confusing, took me a while to get over 😆 however I much prefer descending.

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.

Support for changing sorting direction
3 participants