-
-
Notifications
You must be signed in to change notification settings - Fork 155
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
feat: --json --list-sessions #665
Conversation
Any thoughts on the general design? |
Hey @henryiii sorry this one slipped by me! I like it and was actually surprised at how little needed to change to support this. For those wondering how it ends up looking: Just a couple of questions more than anything. When sessions don't have a python the output from this is I wonder if there's something more helpful we can put here? This seems to be when we use the report.append(
{
"python": session.func.python or f"{sys.version_info.major}.{sys.version_info.minor}",
}
) Finally a very minor one but I'm not sure of the usefulness of the name Other than that though this looks great! |
This can be used to detect when the Python version doesn't matter. I'd assume a user would want to fill nulls with I wouldn't want to inject the actual running Python because you lose the info that it doesn't matter, and you might not be running the listing on the same Python as you run the check later - that shouldn't matter.
I wasn't sure what to call it and being consistent seemed the simplest thing. |
Ahh yeah good point re. "python doesn't matter"
That's fair, naming is hard after all! None of this was a blocker for me, this looks great 👍🏻 |
That doesn't mean it has to be named "call_spec", I just was explaining where the name came from. :) If you want to suggest something, go ahead. |
Well I'm in a similar boat to be honest, I can't really think of a good name either 👀 |
When dumping the JSON is it worth prettying it with |
I considered it, in fact I did do that at first, but dropped it when it's so easy to do it with jq, and the point of the output is to be processed, not to be human readable. But this is not something where saving a few whitespace characters matters, so indent is fine too. My next plan is to try this on a repo somewhere, to see what processing this looks like. I've fixed my blog post's example something like 3-4 times because I missed things. That will probably make good documentation, too. :) |
Yeah agreed, I think it's fine to leave it out. Even if people pipe it to files as soon as they open them in e.g. VSCode it'll probably format it nicely anyway.
This would be great! If you can find some useful tricks it might be worth making a new recipe in the Cookbook I'd be interested to see how people might use this actually, it feels like it could be quite powerful in e.g. CI systems but I can't quite think of a use case in anything I'm doing at the moment |
Where did you get to on this @henryiii? With some tests this should be ready to go as far as I can tell?, would be great to get this in for the next release :) |
Finally tried this out: scikit-build/scikit-build-sample-projects#31 :) |
Signed-off-by: Henry Schreiner <[email protected]>
73316fb
to
f79ce9c
Compare
Signed-off-by: Henry Schreiner <[email protected]>
ebad9ec
to
3d1809c
Compare
Okay, unless someone wants something like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks @henryiii
Closes #658.
No tests yet. Putting out an initial design.
Here's the output on nox itself:
nox --json -l -s cover lint | jq
And it can be processed by jq:
Thoughts?