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

Logger filename fix #24

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

Logger filename fix #24

wants to merge 3 commits into from

Conversation

tricktrap
Copy link
Member

I've noticed that the log filenames are kind of wonky, the months are zero-indexed and none of the fields are padded so lexicographical sorting isn't currently possible. This adds a utility method to generate log filenames based on a calendar entry, and uses it in the log filename generation, and also tests the new functionality.

This uses a date time formatter to fix a couple of issues, mainly
that we were using zero-based months (leading to values of 1 for
January, for example) and also not padding fields, leading to
incorrect lexical sorting.
@codeclimate
Copy link

codeclimate bot commented Feb 22, 2022

Code Climate has analyzed commit d186e00 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 66.6% (50% is the threshold).

This pull request will bring the total coverage in the repository to 18.3% (0.6% change).

View more on Code Climate.

@codecov
Copy link

codecov bot commented Feb 22, 2022

Codecov Report

Merging #24 (d186e00) into master (cee37a9) will increase coverage by 0.58%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #24      +/-   ##
============================================
+ Coverage     17.78%   18.37%   +0.58%     
- Complexity       17       18       +1     
============================================
  Files            13       13              
  Lines           371      370       -1     
  Branches         25       25              
============================================
+ Hits             66       68       +2     
+ Misses          305      302       -3     
Impacted Files Coverage Δ
src/main/java/frc/robot/logging/Logger.java 2.24% <66.66%> (+2.24%) ⬆️

Impacted file tree graph

@TheBitEffect
Copy link
Contributor

When I refactored the code last year, I didn't notice that months were zero-indexed unlike everything else. My fault. One thing that did get cut, though, was adding the op mode to the filename, as I had moved generation of the filename out of the robot file. It might be a good idea to readd that back in. (Maybe as an optional parameter?)

@kryllyxofficial01
Copy link
Contributor

kryllyxofficial01 commented Jul 19, 2022

I've noticed that the log filenames are kind of wonky, the months are zero-indexed and none of the fields are padded so lexicographical sorting isn't currently possible. This adds a utility method to generate log filenames based on a calendar entry, and uses it in the log filename generation, and also tests the new functionality.

I fixed the month index in the new logger system.

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