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 option to control the order of the days in the log output #369

Merged
merged 4 commits into from
Jun 18, 2020

Conversation

joelostblom
Copy link
Contributor

I think it is quite common to sort with the newest entry at the bottom (e.g with ls and this is also how the times are ordering within each day in the log output), so I suggest adding an option to enable this behavior.

Copy link
Contributor

@jmaupetit jmaupetit left a comment

Choose a reason for hiding this comment

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

Having a global configuration for this is a good idea, but I also would like to be able to override it while calling the log command. So, adding an extra option to the aforementioned, would be lovely.

@joelostblom
Copy link
Contributor Author

Will add a flag with the same name --log_latest_day_on_top!

@jmaupetit
Copy link
Contributor

Why not --reverse?

@joelostblom
Copy link
Contributor Author

That's what I had initially, but then I wanted to be more specific to indicate that it is only reversing the order of the days, not the time entries within the days, so that I would leave it open for someone to add an option to reverse times if desired. This might never be the case though, and I am happy to rename both the option and the flag reverse-log if you prefer that.

@jmaupetit
Copy link
Contributor

--reverse should be fine, thanks!

Copy link
Contributor

@jmaupetit jmaupetit left a comment

Choose a reason for hiding this comment

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

LGTM @joelostblom 🙏

Do you think you can add a test for this?

@joelostblom
Copy link
Contributor Author

I can try, but might need some assistance to make it satisfactory. Before giving it a go, I just want to clarify what what you would like this test to contain. Do you want it to be something simple like the current CLI log tests, just adding the --reverse flag and check that no errors are raised? Or do you want there to be a new log section in test_watson.py which checks the actual ordering of the days when using --reverse/--no-reverse?

@jmaupetit
Copy link
Contributor

Testing Watson's API is easier to implement, so adding a new log section in test_watson.py would be perfect.

@joelostblom joelostblom force-pushed the log-latest-day-on-top branch from d88b64f to b4d99a1 Compare May 16, 2020 20:24
@joelostblom
Copy link
Contributor Author

@jmaupetit Let me know what you think of the tests I just added. Since log is defined entirely in cli.py, I could not figure out a way to test it in watson.py. I tried adding it in with the other cli-tests but could not figure out a good way to add data and test it, so I added the tests to test_utils.py. The disadvantage is that we're really only testing the inner most function and things could of course go wrong outside, but I wasn't able to implement anything better. Let me know if you have suggestions. The failing flake8test is unrelated and fails on master as well.

@joelostblom
Copy link
Contributor Author

@jmaupetit Just a reminder to have a look at this when you get a chance.

jmaupetit
jmaupetit previously approved these changes Jun 17, 2020
Copy link
Contributor

@jmaupetit jmaupetit left a comment

Choose a reason for hiding this comment

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

@jmaupetit
Copy link
Contributor

Thanks @joelostblom ! If you handle the rebase, I'll merge this asap 🙏

@joelostblom
Copy link
Contributor Author

Haha thanks! =) I just rebased and squashed a few of the commits.

Also fix previous changelog typo
@joelostblom
Copy link
Contributor Author

Realized that the changelog entry was in the wrong place since the 1.9 tag was added recently. Ready for merge now 👍

@jmaupetit jmaupetit merged commit ac8a23c into jazzband:master Jun 18, 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)
@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.

2 participants