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 config option for the length of watson log #436

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

joelostblom
Copy link
Contributor

@joelostblom joelostblom commented Jun 5, 2021

I think the default of showing the past 7 days is rather long and usually I only need to see the last few days to know if I need to use restart for a previous entry. I modified the code a few months back to show only the last 3 days on my own installation, but figured I might as well make it a config options so others can use it. This is particularly useful if you have many entries in each day and don't want to have to scroll in your parser every time you run the command and even better if using less -X -F which only opens less when the output is longer than the screen height.

Personally I don't think there needs to be a new flag added, since there there are already several flags to control the log command from the CLI, just nothing for the default behavior.

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.

Very good idea 👍 .

Do you think you could add a test for it in test_cli ?

@joelostblom
Copy link
Contributor Author

Thanks @k4nar ! I added an attempt to a test. I am not sure whether we need to parameterize the test with multiple dates or if it is enough to just check that the log length is correct since it should be invariant of the current date. When I tried to parameterize with multiple dates I also couldn't figure out how to get log to count from the parameterized date since it uses arrow.now() internally. Overriding the now return value didn't seem to work and it's late now =) Let me know if you think this is sufficient.

@joelostblom
Copy link
Contributor Author

@k4nar Just a reminder to review this when you get a chance!

@joelostblom
Copy link
Contributor Author

@k4nar Just a ping to review this when you get a chance!

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.

3 participants