Skip to content

Conversation

@matthewfeickert
Copy link
Member

@codecov
Copy link

codecov bot commented Nov 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.51%. Comparing base (df3dbc8) to head (d9005a6).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #195   +/-   ##
=======================================
  Coverage   72.51%   72.51%           
=======================================
  Files           4        4           
  Lines         131      131           
=======================================
  Hits           95       95           
  Misses         36       36           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@matthewfeickert
Copy link
Member Author

cc @henryiii for review as he was the last to touch this segment of the README and added the CLI options in PR #51.


* uv:

```
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
```
```bash

Copy link
Member Author

Choose a reason for hiding this comment

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

Bash syntax highlighting on GitHub looks the same as no syntax highlighting on GitHub if you aren't using explicit Bash commands. I would normally suggest console, as that's more correct as people will be executing these in shells regardless of shell type, but that does change the syntax highlighting in a way that just converts all of it to a new color, which isn't very useful.

Is there an advantage to selecting bash here? If no, should we change Scikit-HEP style recommendations?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess none of the three options are much better than the others.

skhep-testdata --all --dir local
skhep-testdata --all --dir local
skhep-testdata --all --dir local

But to be consistent with the rest of the readme, it would be good to keep using bash or switch everything else to either of the other options.

Copy link
Member

@APN-Pucky APN-Pucky Nov 24, 2025

Choose a reason for hiding this comment

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

Besides consistency, it also adds context for LLMs even if it does not change visually on Github. I don't think sh vs bash vs console matters, except that bash seems more common. Maybe all should be changed to console then?

Copy link
Member Author

@matthewfeickert matthewfeickert Nov 26, 2025

Choose a reason for hiding this comment

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

Nothing:

skhep-testdata --all --dir local

bash:

skhep-testdata --all --dir local

console:

skhep-testdata --all --dir local

@ariostas @APN-Pucky I think that this demonstrates why trying to apply a blanket default to every code block is not a reasonable approach. bash means Bash code. A tool CLI is not Bash, and so the local is "correctly" incorrectly highlighted, as local is valid Bash keyword. console is a different color and looks strange because it is expecting to display a command at the prompt and the output, as can be seen in this example

$ skhep-testdata --all --help
usage: skhep_testdata [-h] [--all] [--list] [--dir DIR] [file_path ...]

Expand a testing dataset path to a full path, downloading things if a remote file is request.

positional arguments:
  file_path             Path to expand

options:
  -h, --help            show this help message and exit
  --all                 Download all files
  --list                List all files
  --dir DIR             Path to store in (DEFAULT: ~/.local/skhepdata)

Besides consistency, it also adds context for LLMs even if it does not change visually on Github

I do not find this to be a good argument. We should not be changing how we write human documentation for LLMs — that seems the wrong way round in terms of adaptation. It is reasonable to write different forms of docs if you want to cater to LLMs, but it would also provide incorrect context to label things as bash or console when they're not.

But to be consistent with the rest of the readme, it would be good to keep using bash or switch everything else to either of the other options.

I'd prefer to move to removing syntax hints that do not correspond to the actual language in the block. Though if others would prefer other options I'd suggest moving this to a GitHub Issue for additional discussion and following up on the mailing list for GitHub org wide agreement.


* pipx:

```
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
```
```bash


* Pixi:

```
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
```
```bash

Copy link
Member

@eduardo-rodrigues eduardo-rodrigues left a comment

Choose a reason for hiding this comment

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

The changes seem good to me.

Copy link
Collaborator

@ariostas ariostas left a comment

Choose a reason for hiding this comment

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

The changes also look good to me. I left a comment in the above discussion, but it's a minor thing, so I'll approve it already.

@matthewfeickert
Copy link
Member Author

Sorry for being slow here. As @APN-Pucky and @ariostas gave 👍 to #195 (comment) I'm going to merege this. If people would like to have additional discussion on Markdown that's great by me, but please open a new issue and let's follow up there. 👍

* As installation creates the executables 'bin/scikit-hep-testdata' and
  'bin/skhep-testdata', demonstrate these in the README rather than running
  the module as a script with '-m'.
   - c.f. https://docs.python.org/3/using/cmdline.html#cmdoption-m
* Show how to use 'uvx', 'pipx', and 'pixi exec' to be able to access and use
  the CLI API without having to explicitly install into an environment.
@matthewfeickert matthewfeickert merged commit 606c561 into scikit-hep:main Dec 7, 2025
7 checks passed
@matthewfeickert matthewfeickert deleted the docs/use-cli branch December 7, 2025 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants