Skip to content
This repository has been archived by the owner on Sep 6, 2018. It is now read-only.

Check bisq.asset.Asset file sorting during CI #11

Merged
merged 4 commits into from
May 15, 2018

Conversation

blabno
Copy link

@blabno blabno commented May 15, 2018

(edited by @cbeams):

This PR is to deal with several ordering issues that crept into the file over time. So far we've only been eyeballing sort order during PR review, and we missed a few items, so now is a good time to automate checking the sort order via Travis CI.

@blabno blabno force-pushed the bugfix/fix-asset-sorting branch 3 times, most recently from 9a0fd0c to a499cd2 Compare May 15, 2018 10:29
@blabno blabno force-pushed the bugfix/fix-asset-sorting branch from a499cd2 to 062c6c7 Compare May 15, 2018 10:31
Copy link
Contributor

@cbeams cbeams left a comment

Choose a reason for hiding this comment

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

ACK. You'll see a few additional commits I've pushed, @blabno. It's a shame how unpredictable the behavior of sort is across machines. I was never able to figure out LC_COLLATE, LC_CTYPE or LC_ALL values that got everything to work consistently. Bit of a hack in the end grepping out the comments, but it works. Thanks for putting this together.

@cbeams
Copy link
Contributor

cbeams commented May 15, 2018

@blabno, note that I updated the description of the PR as well. In general, even if you and I have just discussed something in Slack (as we had in this situation), even a one-line description in the PR is good practice. It provides context for the uninitiated, especially those who might come along and do reviews. As time goes by we want to get more and more in the culture / mode where anybody who wants to can do a review. This means that PRs should be as self-contained as they can be, without requiring context from Slack or wherever else.

No big deal—just want to start emphasizing this. Really, just a one liner is fine, whatever you think would help an uninitiated person understand why this PR is being submitted, why now, etc.

@cbeams cbeams merged commit 8e03ceb into bisq-network:master May 15, 2018
@blabno blabno deleted the bugfix/fix-asset-sorting branch May 15, 2018 11:42
@tearodactyl tearodactyl mentioned this pull request May 21, 2018
@cbeams cbeams mentioned this pull request May 30, 2018
5 tasks
@ghost ghost mentioned this pull request Aug 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants