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

Script for checking codeblock coverage #2731

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

falOn-Dev
Copy link

I wrote a simple tool for getting data on how many codeblocks cover each language. By default it will check for Python, C++, and Java, but the RegEx is generated at runtime based on what languages you pass in, so it still works for checking the coverage of other languages.

This script contains its own documentation on how to run it, but I'm happy to add a page to the frc-docs contributing guide if needed.

(I made a similar PR earlier but I wanted to make a new one as that one contains differing information from this newer version of the script)

Copy link
Contributor

@spacey-sooty spacey-sooty left a comment

Choose a reason for hiding this comment

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

image
Uhhh?

Copy link
Contributor

@spacey-sooty spacey-sooty left a comment

Choose a reason for hiding this comment

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

Could you add an option to link to the non covered parts?

scripts/check_codeblock_coverage.py Show resolved Hide resolved
@falOn-Dev
Copy link
Author

Could you add an option to link to the non covered parts?

That's what --verbose does, unless I'm misunderstanding?

@spacey-sooty
Copy link
Contributor

Should check for when no arguments are provided and display help

@spacey-sooty
Copy link
Contributor

Could you add an option to link to the non covered parts?

That's what --verbose does, unless I'm misunderstanding?

Verbose just breaks it for me
image

@falOn-Dev
Copy link
Author

Ah, it's working it just doesn't say anything, okay I'll fix both of those things

@TheTripleV
Copy link
Member

I'm concerned about this script.

  1. It might be better as a one off tracking issue listing everything missing
  2. As there are unique features for each language (coroutines in progress for python and java, epilogue for java), not all code blocks should contain all 3 languages.
  3. If this ends up being a ci check, I don't want prs to fail because the author doesn't know cpp. Separate prs for each lang are ok.
  4. I'd be more comfortable if it was implemented with Sphinx's api instead of regex.

@falOn-Dev
Copy link
Author

Implemented both of @spacey-sooty requests.

@falOn-Dev
Copy link
Author

I'm concerned about this script.

1. It might be better as a one off tracking issue listing everything missing

2. As there are unique features for each language (coroutines in progress for python and java, epilogue for java), not all code blocks should contain all 3 languages.

3. If this ends up being a ci check, I don't want prs to fail because the author doesn't know cpp. Separate prs for each lang are ok.

4. I'd be more comfortable if it was implemented with Sphinx's api instead of regex.

I do not think this should be a CI thing, or at least it shouldn't fail PRs. It's intended to be a tool for people to just check if there are any glaring gaps in language support. I can understand wanting it in Sphinx's API but I have never used that, so I used regex, if it's absolutely necessary to not be regex that's fine

@spacey-sooty
Copy link
Contributor

spacey-sooty commented Sep 10, 2024

3. If this ends up being a ci check, I don't want prs to fail because the author doesn't know cpp. Separate prs for each lang are ok.

I feel like this is a recipe to have documentation fall further behind for Python and make C++ start falling behind substantially. I can easily see a PR getting through with C++ to be done later then it never being done (its happened before)

@falOn-Dev
Copy link
Author

I feel like if the tool has proper warnings (which I'm happy to add more of) then not putting it in over the possibilities of misuse would be a bad decision, because it would still be useful

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