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

More stats #408

Merged
merged 10 commits into from
Nov 23, 2024
Merged

More stats #408

merged 10 commits into from
Nov 23, 2024

Conversation

mzuenni
Copy link
Collaborator

@mzuenni mzuenni commented Nov 12, 2024

more stats (mostly useful as fun stats for solution slides)

@mzuenni mzuenni requested a review from mpsijm November 12, 2024 15:00
@RagnarGrootKoerkamp
Copy link
Owner

Can you improve the installation instructions for pygount? I don't have it and there's no pacman/AUR package.. Did you install it in your user environment with pip or so?

@mzuenni
Copy link
Collaborator Author

mzuenni commented Nov 15, 2024

yes installed it with pip... we could also use non python code to do the counting...

@mpsijm
Copy link
Collaborator

mpsijm commented Nov 15, 2024

Or do the counting manually in Python? Iterating over files in directories and discarding comments in 5 languages shouldn't be too much code 🙂

@mzuenni
Copy link
Collaborator Author

mzuenni commented Nov 15, 2024

I would say counting this ourselves is not an option... and just for the fun, how many c++ lines of code are this:

//this is a comment\
is this a comment?
int main() {}

cloc says 2 where as pygount says 1...

@mzuenni
Copy link
Collaborator Author

mzuenni commented Nov 16, 2024

any idea for more stats that we want to calculate?

@mzuenni
Copy link
Collaborator Author

mzuenni commented Nov 17, 2024

@RagnarGrootKoerkamp the code now only requires pygments and not pygount, is that easier to install?

@mpsijm
Copy link
Collaborator

mpsijm commented Nov 17, 2024

Fixed the rebase, it looked like all commits from master ended up in this branch without the base actually changing, so the commits were duplicated 😛 But not anymore 😄

I can confirm that it works on my machine, I already had python-pygments installed via Pacman. I like the approach of reusing the Pygments lexer data for counting the number of lines of code 😄

I'm not sure if --slides is the best name for the flag, because it doesn't actually do anything with slides (the fact that we may use these stats for the slides, is something else). If we want to name the flag for the thing that it does (namely analyzing the submissions, test data, and Git repo), some suggestions could be --more-analysis, --more-stats, --more, --analyse-data, or --analyse-repo (not sure about - vs. _). Maybe we could (ab)use --verbose to trigger these extra stats, but that also feels slightly odd.

@mzuenni
Copy link
Collaborator Author

mzuenni commented Nov 17, 2024

Fixed the rebase, it looked like all commits from master ended up in this branch without the base actually changing, so the commits were duplicated 😛 But not anymore 😄

whoops and thanks ^^

I'm not sure if --slides is the best name for the flag

hm simply --more or --all instead?

@mpsijm
Copy link
Collaborator

mpsijm commented Nov 18, 2024

Hmm, --all also has a different meaning for other commands already 🤔 But I'm fine with --more 🙂

@RagnarGrootKoerkamp
Copy link
Owner

RagnarGrootKoerkamp commented Nov 19, 2024

Tested the latest version and LGTM, so feel free to merge. Haven't checked the latest code in detail.

@@ -21,6 +21,8 @@ repos:
- ruamel.yaml==0.18.6
- questionary==2.0.1
- types-colorama==0.4.15.20240311
- types-Pygments==2.18.0.20240506
- types-python-dateutil==2.9.0.20241003
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not datetime.strptime (https://stackoverflow.com/a/2265383) to avoid an additional dependency?

Also, this/these additional (optional) dependenc(y/ies) should be mentioned in other places, I think.

@mpsijm
Copy link
Collaborator

mpsijm commented Nov 19, 2024

Didn't check the code inside more_stats, but that should be fine since it's behind the --more flag and it works 😛 For the code around it, I only have this above comment about dependencies 🙂

@mzuenni mzuenni merged commit 09bf263 into master Nov 23, 2024
4 checks passed
@mzuenni mzuenni deleted the more-stats branch November 23, 2024 15:28
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