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

Fix tests for arrow>=0.15.6 #380

Merged
merged 1 commit into from
Jun 10, 2020
Merged

Fix tests for arrow>=0.15.6 #380

merged 1 commit into from
Jun 10, 2020

Conversation

oxzi
Copy link
Contributor

@oxzi oxzi commented Jun 7, 2020

Since its last release0, the heavily used arrow library supports ISO
week dates1. The CLI tests assumed that arrow does not support this.
However, the new version nullified this assumption.

As a provisional measure, the version of arrow was limited upwards in
PR #3722. An obsolete version was thereby requested.

A current version, or at least support for one, is important for
third-party package managers. Especially GNU/Linux distributions prefer
to use their own package manager to install software over pip.

Thus, this commit removes both the restriction to an outdated arrow
version in the requirements.txt and validates previously invalid marked
week dates.

@jmaupetit
Copy link
Contributor

It's a strong requirement, but I've no hard-feelings about this. @k4nar & @joelostblom WDYT?

@oxzi
Copy link
Contributor Author

oxzi commented Jun 8, 2020

Currently I see three options:

  1. Enforce an outdated version of arrow and keep the old tests.
  2. Enforce the latest version of arrow and use the updated tests.
  3. Remove the ISO week date-test and a limitation to arrow's version.

Imho, removing tests is never good. Thus, one has to pick a side of arrow's version barrier: use an old version or force the (currently) newest.

@joelostblom
Copy link
Contributor

I don't have anything against this either. I am not very experienced with what Linux distros prefer so I defer judgement there (although my general problem with many have been that they have too old packages so not sure if they would have such a recent version of arrow).

One additional alternative could be to have a separate version requirement on arrow for development, since it only breaks the tests and list in the docs that week dates are only available on the latest version of arrow, which would avoid limiting the availability of Watson to those that don't care about week dates or running the tests, but for some reason can't get the latest arrow (maybe this is not at all common and therefore moot).

Just throwing out ideas.

@oxzi
Copy link
Contributor Author

oxzi commented Jun 9, 2020

I created this PR as a contributor to a distribution - NixOS, if it matters. The latest arrow version thus prevented me from version bumping watson. As options I saw adding a custom patch, submitting a patch upstream or using an extra arrow package only for watson.

Others are facing the same problem, e.g., openSUSE have patched watson to remove arrow's version limitation. Nevertheless, it's dark out there regarding packages - both for arrow and watson.

Yet enforcing an old version leads to incompatibilities in the long run. Incentives for updating should therefore be provided.

@jmaupetit
Copy link
Contributor

@geistesk you convinced me 😉 Waiting for @k4nar opinion on this.

k4nar
k4nar previously approved these changes Jun 9, 2020
Copy link
Collaborator

@k4nar k4nar left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 .

@jmaupetit
Copy link
Contributor

Ok, then. @geistesk can you update the changelog so that I can merge this. Thank you! 🙏

@oxzi
Copy link
Contributor Author

oxzi commented Jun 9, 2020

@jmaupetit: Does it look right to you now?

Edit: Sorry for the second modification, I rephrased it a little.

Since its last release[0], the heavily used arrow library supports ISO
week dates[1]. The CLI tests assumed that arrow does not support this.
However, the new version nullified this assumption.

As a provisional measure, the version of arrow was limited upwards in
PR jazzband#372[2]. An obsolete version was thereby requested.

A current version, or at least support for one, is important for
third-party package managers. Especially GNU/Linux distributions prefer
to use their own package manager to install software over pip.

Thus, this commit removes both the restriction to an outdated arrow
version in the requirements.txt and validates previously invalid marked
week dates.

[0]:https://github.com/crsmithdev/arrow/releases/tag/0.15.6
[1]:https://en.wikipedia.org/wiki/ISO_week_date
[2]:jazzband#372
@jmaupetit jmaupetit merged commit 2e973c2 into jazzband:master Jun 10, 2020
@jmaupetit
Copy link
Contributor

Thank you @geistesk 🙏

@oxzi oxzi deleted the arrow-0.15.6 branch June 22, 2020 10:58
jmaupetit added a commit that referenced this pull request Jul 3, 2020
Added:

- Log output order can now be controlled via the `--reverse/--no-reverse` flag
  and the `reverse_log` configuration option (#369)
- Add `--at` flag to the `start` and `restart` commands (#364).
- Add `--color` and `--no-color` flags to force output to be colored or not
  respectively (#350).

Changed:

- Require latest Arrow version 0.15.6 to support ISO week dates (#380)

Fixed:

- Make after-edit-check ensure that edited time stamps are not in the future
  (#381)
@jmaupetit jmaupetit mentioned this pull request Jul 3, 2020
jmaupetit added a commit that referenced this pull request Jul 3, 2020
Added:

- Log output order can now be controlled via the `--reverse/--no-reverse` flag
  and the `reverse_log` configuration option (#369)
- Add `--at` flag to the `start` and `restart` commands (#364).
- Add `--color` and `--no-color` flags to force output to be colored or not
  respectively (#350).

Changed:

- Require latest Arrow version 0.15.6 to support ISO week dates (#380)

Fixed:

- Make after-edit-check ensure that edited time stamps are not in the future
  (#381)
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.

4 participants