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

Log results to platform specific location #3

Merged
merged 5 commits into from
Apr 28, 2022
Merged

Log results to platform specific location #3

merged 5 commits into from
Apr 28, 2022

Conversation

nickwanninger
Copy link
Contributor

What: Logging historical data to $XDG_DATA_HOME/thokr.log

Why: So nerds like me can plot progress over time

How: Results are logged whenever results are calculated. It may be smart to add a configuration to disable this, but that could be another comit

Checklist: N/A

  • Documentation added
  • Tests added
  • No linting errors / broken tests
  • Merge ready

Copy link
Owner

@jrnxf jrnxf left a comment

Choose a reason for hiding this comment

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

Left a few comments, overall a big fan of the idea!

src/thok.rs Outdated
@@ -68,6 +71,33 @@ impl Thok {
}
}

pub fn save_results(&self) -> io::Result<()> {
let log_path = dirs::data_dir().unwrap().join("thokr.log");
Copy link
Owner

Choose a reason for hiding this comment

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

Given the fact that we'd be exporting the data in a csv format already, I'd prefer we just name the file [filename].csv!

Copy link
Owner

Choose a reason for hiding this comment

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

Also I think I'd rather thokr be a folder and this file be nested inside, that way there's already a folder for potential more files down the road

Copy link
Owner

Choose a reason for hiding this comment

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

Just checking that there should be no issues with dirs and windows? I'm extremely unfamiliar with windows os lol. Do they have a XDG_DATA_HOME?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree with the folder idea. $XDG_DATA_HOME/thokr/log.csv?

Also yeah I think that dirs is good on windows (at least they say so haha)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed this change

@@ -141,6 +171,8 @@ impl Thok {
self.wpm = 0.0;
}
self.accuracy = ((correct_chars.len() as f64 / self.input.len() as f64) * 100.0).round();

let _ = self.save_results();
Copy link
Owner

Choose a reason for hiding this comment

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

Any reason for the let _?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the least gross way to ignore an error. It could be replaced with .ok() or .drop().

I think it makes sense to ignore the error here, as failing to log results shouldn't crash the program. Maybe instead it should log to stderr or display on the results screen?

src/thok.rs Outdated
.open(log_path)?;

if needs_header {
writeln!(log_file, "# Date, WPM, Accuracy, Standard Deviation")?;
Copy link
Owner

Choose a reason for hiding this comment

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

Not a big fan of # Date -- mostly just the # feels confusing. I'd want to add a few more headers here too, namely to track what parameters the test was run with. Was it 15 words? 100? Was it a 30 second test? 60? Stuff like that!

Copy link
Contributor Author

@nickwanninger nickwanninger Apr 27, 2022

Choose a reason for hiding this comment

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

I'm happy to add that for sure. I'm not the most familiar with the project, so what might be the best way to get the word count other than counting the ' ' chars in self.prompt?

src/thok.rs Outdated Show resolved Hide resolved
nickwanninger and others added 2 commits April 26, 2022 22:17
Co-authored-by: colby thomas <coloradocolby@gmail.com>
@nickwanninger
Copy link
Contributor Author

It should now log with the wordcount and duration. I modified the Thok structure to include a test_duration, and renamed the existing duration to time_remaining to clarify things a bit

@jrnxf jrnxf added the enhancement New feature or request label Apr 27, 2022
@jrnxf
Copy link
Owner

jrnxf commented Apr 28, 2022

Will cut a release in a bit, but merging for now! Thanks for the hard work @nickwanninger !

@jrnxf jrnxf merged commit d2879e6 into jrnxf:main Apr 28, 2022
@nickwanninger
Copy link
Contributor Author

For sure! Happy to help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants