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

[proposal] basic ci #41

Closed
ghost opened this issue Apr 8, 2021 · 8 comments
Closed

[proposal] basic ci #41

ghost opened this issue Apr 8, 2021 · 8 comments

Comments

@ghost
Copy link

ghost commented Apr 8, 2021

A folder that expects structure (see #40) would benefit from basic CI.

Before embarking, should we add CI? If so, what should CI check?

ideas

  • lint trailing whitespace from .nu files
  • reasonability tests? Many one liners expect context, but can nu do static analysis?
@fdncred
Copy link
Collaborator

fdncred commented Apr 8, 2021

I'm fine with a two phased approach.
Phase 1: Add scripts
Phase 2: CI

I'm just not sure what we can test. I'm not sure I care about extra spaces. I mean, it would be nice to trim the lines of trailing whitespace but I'm not sure it's critical.

I'd really like to test every script so that when CI fails, we know that nushell changed. For instance, recently we changed count to length. Those scripts that still have count should fail on the latest main. But we can't test all scripts. Maybe we should just have a test suite file where we add particular files to one master file that can be tested without outside intervention?

@fdncred
Copy link
Collaborator

fdncred commented Apr 8, 2021

I also like your idea to give credit where it's due on one-liners. Good job!

@ghost
Copy link
Author

ghost commented Apr 12, 2021

I mean, it would be nice to trim the lines of trailing whitespace but I'm not sure it's critical.

Agreed. I seek out errant whitespace to remove it. So entirely personal.

But we can't test all scripts. Maybe we should just have a test suite file where we add particular files to one master file that can be tested without outside intervention?

This is a great point.
I think establishing what we can test and how is critical.
It seems to me that two broad categories emerge for oneliners: self-contained and those with external dependencies.

I also feel like that connects to the problem of scripts for learning and teaching versus those for experts who know what they are looking at.

If we establish a convention for learning oneliners, we could better test them.

Maybe we should just have a test suite file where we add particular files to one master file that can be tested without outside intervention?

I love the idea to collect them into one so we can test and do more things with them.
But I would prefer a way to codify this by where we put the script or some kind of opt-in annotation in a comment.

@fdncred
Copy link
Collaborator

fdncred commented Apr 12, 2021

I love the idea to collect them into one so we can test and do more things with them.
But I would prefer a way to codify this by where we put the script or some kind of opt-in annotation in a comment.

I'm open to ideas but I was thinking something like text files.

self-contained.txt

script1.nu
script2.nu
script3.nu

external.txt

script4.nu
script5.nu
script6.nu

And then we need to get nushell somehow in the CI and run the self-contained scripts looking for failures.
The external scripts could be tested via a container given we have the necessary requirements like input.json or some_databaseor whatever, but that may be too big of a leap.

I prefer starting very simple and then submitting PRs over time to add complexity where it's necessary.

@ghost
Copy link
Author

ghost commented Apr 12, 2021

Sounds great!

And then we need to get nushell somehow in the CI and run the self-contained scripts looking for failures.

Random but I recently implemented nu scripting in a project so I can provide one answer to this question.
In an Alpine edge environment:

apk add nushell --update-cache --repository http://dl-3.alpinelinux.org/alpine/edge/testing/ --allow-untrusted

I needed to use alpine:edge because of nushell's git dependency.

@ghost
Copy link
Author

ghost commented May 19, 2021

I noticed some scripts are marked as executable and others are not.
Perhaps we could consider linting that user-facing scripts are marked as executable.

@fdncred
Copy link
Collaborator

fdncred commented May 19, 2021

@efx i'm on windows and none of them are marked as executable since that isn't a thing. :) For *nix I'm happy to accept a PR that does that or linting as you suggest.

@ghost
Copy link
Author

ghost commented May 19, 2021

@fdncred ah, thanks for noting that; corrects my perspective on whether it is worth pursuing. My *nix bias creeping in 😄.

@fdncred fdncred closed this as completed Jan 8, 2022
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

No branches or pull requests

1 participant