Skip to content

Add reverse sort ability, __repr__ and bump version#2

Merged
usmanm merged 6 commits intomasterfrom
order
Jul 14, 2014
Merged

Add reverse sort ability, __repr__ and bump version#2
usmanm merged 6 commits intomasterfrom
order

Conversation

@usmanm
Copy link
Copy Markdown
Member

@usmanm usmanm commented Jul 12, 2014

Adds an optional keyword argument reverse to TimeUUIDs constructor. False by default, but if set to True it will reverse the comparison operation.

@usmanm usmanm changed the title Add reverse sort ability Add reverse sort ability and __repr__ Jul 13, 2014
@usmanm usmanm changed the title Add reverse sort ability and __repr__ Add reverse sort ability, __repr__ and bump version Jul 13, 2014
@marcua
Copy link
Copy Markdown
Contributor

marcua commented Jul 14, 2014

this lgtm, but the reverse=True stuff is confusing. Can we switch to ResultOrder.ASCENDING/ResultOrder.DESCENDING?

If not, at least change reverse to ascending or descending depending on which it is. I like that less, but it's an easier change and better than reverse imo

Comment thread timeuuid/timeuuid.pyx Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

docstring on what reverse does. Also consider changing the name as per comment above :)

@usmanm
Copy link
Copy Markdown
Member Author

usmanm commented Jul 14, 2014

I don't want to leak kronos level stuff into TimeUUID. I'll change it to descending.

Comment thread timeuuid/timeuuid.pyx Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"optional, False by default, which sorts in ascending order"

@marcua
Copy link
Copy Markdown
Contributor

marcua commented Jul 14, 2014

change comment and them lgtm---land when ready

usmanm added a commit that referenced this pull request Jul 14, 2014
Add descending sort ability, __str__ and bump version
@usmanm usmanm merged commit 04c28de into master Jul 14, 2014
@usmanm usmanm deleted the order branch July 14, 2014 18:13
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.

2 participants