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

Use reorder instead of order in #latest method #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

OrelSokolov
Copy link
Contributor

Checklist

  • Continuous integration passes, or no functional code was changed
  • Changelog entry added, or no entry is necessary

Notes

@izzergh
Copy link
Contributor

izzergh commented Jan 14, 2022

I hesitate using reorder because that removes the entire ORDER clause from the sql, and replaces it with just ORDER BY start_date DESC. That works for your specific case, sure, but I would consider this unexpected behavior in general.

This seems an appropriate time to use a ! - add another method latest! (and earliest!) that uses reorder instead of order. And document it (currently all user documentation is in the readme, so that documentation would go there).

Also, per the PR template, needs a changelog entry.

Also @oneamtu looks like Travis didn't run on this branch, FYI. I may look into it later if you don't beat me to it - I think you have the credentials though in case it needs some configuration on the Travis side.

@oneamtu
Copy link
Contributor

oneamtu commented Jan 14, 2022

Yup @OrelSokolov what Isaac said, reorder clobbers the existing order; I get it that w/ latest you probably want that to happen. Or raise a warning if there is an existing order, that's ok too (i.e. at least the user knows).

@oneamtu
Copy link
Contributor

oneamtu commented Jan 14, 2022

For TravisCI, seems like they deprecated their old open-source options - https://docs.travis-ci.com/user/migrate/open-source-repository-migration, so we'll have to set it up. We can migrate, but also potentially a good time to check out other options - github actions may be easier, we use CircleCI but also curious about gitlab CI/CD since it seems like a good option generally for our org - https://about.gitlab.com/solutions/github/. Any thoughts @izzergh, @DavidA001?

@OrelSokolov OrelSokolov mentioned this pull request Jan 25, 2022
2 tasks
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