chore: update sprocket to latest WDL crate #46

a-frantz · 2024-12-17T22:51:16Z

This also adds the ability to check/lint URLs

closes #39
sort of closes #45 by providing --local-only and --single-document args which narrow the scope of printed diagnostics. It was decided over slack not to pursue either of the 2 fixes presented in #45 as they were likely undesirable UXs.

Before submitting this PR, please make sure:

You have added a few sentences describing the PR here.
You have added yourself or the appropriate individual as the assignee.
You have added at least one relevant code reviewer to the PR.
Your code builds clean without any errors or warnings.
You have added tests (when appropriate).
You have updated the README or other documentation to account for these changes (when appropriate).

This also adds the ability to check/lint URLs

CHANGELOG.md

a-frantz · 2024-12-18T16:33:33Z

This PR was put back into draft state and will remain as a draft until the wdl crates make an official release. In the meantime, I'll use this branch as the basis for the development of the other sprocket PRs I have planned.

This PR will have to be reviewed+merged before those other ones.

adthrasher · 2025-01-02T15:41:24Z

CHANGELOG.md

@@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 ## Unreleased

+### Changed
+
+* Updated WDL crate to latest. This adds support for


Didn't Clay want to keep sprocket pinned to a release?

Yes, but that release doesn't exist yet. This PR will be updated to pull an official release once one exists.

I think the language here is fine? Maybe it should specify the version of "latest" once that is out?

OK, I would swap latest with the version when it is known.

adthrasher · 2025-01-02T15:44:04Z

src/commands/check.rs

-    if !args.lint && !args.common.except.is_empty() {
-        bail!("cannot specify `--except` without `--lint`");
+    if args.common.shellcheck && !args.lint {
+        bail!("`--shellcheck` requires `--lint` to be enabled");


Is this the experience we want or should --shellcheck add --lint automatically (with a warning)?

I could go either way I suppose. I don't think I feel strongly on this either way. Could it confuse users if they specify --shellcheck without --lint and then they see non-shellcheck lint diagnostics?

adthrasher · 2025-01-02T16:11:27Z

src/commands/check.rs

-            "aborting due to previous {error_count} error{s}",
+            "failing due to {error_count} error{s}",
            s = if error_count == 1 { "" } else { "s" }
        );
    } else if args.common.deny_warnings && warning_count > 0 {
        bail!(
-            "aborting due to previous {warning_count} warning{s} (`--deny-warnings` was specified)",
+            "failing due to {warning_count} warning{s} (`--deny-warnings` was specified)",
            s = if warning_count == 1 { "" } else { "s" }
        );
    } else if args.common.deny_notes && note_count > 0 {
        bail!(
-            "aborting due to previous {note_count} note{s} (`--deny-notes` was specified)",
+            "failing due to {note_count} note{s} (`--deny-notes` was specified)",


I'm not sure about this language change. I'm not sure the new proposed language makes it clear that sprocket is stopping because it has encountered too many errors/notes/warning.

I think that this language is an improvement, but maybe it can be improved further. I don't like the aborting language because in my mind it implies that sprocket was going to do something else but it's quitting early. This really just controls whether it's a non-zero exit code or not.

Can you clarify the behavior here? sprocket is computing the full set of diagnostics, reporting all of them, and then setting the exit code if any of the requested subtypes has content?

You are correct. The bail! macro exits the program with a non-zero exit code (IDK what code specifically) and this block of checks is the last thing sprocket is doing in check/lint mode. So there is no further computation that might occur if there aren't problems detected. This is the end no matter what.

OK, I initially thought this was related to limiting the number of diagnostics emitted to the first N. I know that was discussed, but can't recall what the decision was.

Now that I understand it, I agree that the language is an improvement.

We did discuss that over slack, but it was decided that limiting to the first N would likely lead to a bad UX. So we always report the full set of diagnostics, with --local-only and --single-document controlling the scope of what's reported (but everything is always computed regardless)

Thanks for the refresher, the after-effects of the holiday break are quite apparent today.

a-frantz · 2025-01-02T16:25:45Z

src/commands/check.rs

    #[arg(required = true)]
-    pub paths: Vec<PathBuf>,
+    #[clap(value_name = "PATH or URL")]
+    pub file: String,


A potential nit @adthrasher : is file the right name for this arg? I couldn't think of a better name...

I considered commenting on that, but couldn't come up with an alternative I preferred. You could verbosely call it path_or_url.

Eh. I think I prefer just plain path to path_or_url, but don't love either. I'll leave this comment unresolved in case someone else reviewing has a better alternative.

adthrasher · 2025-01-02T16:40:27Z

src/commands/check.rs

@@ -182,17 +257,17 @@ pub async fn check(args: CheckArgs) -> anyhow::Result<()> {

    if error_count > 0 {
        bail!(
-            "aborting due to previous {error_count} error{s}",
+            "failing due to {error_count} error{s}",


Suggested change

"failing due to {error_count} error{s}",

"failing with {error_count} error{s}",

How do you feel about with instead? I'm not certain that's an improvement.

Hm. Kinda feels like half-dozen of one, 6 of the other to me. I don't see how it clarifies anything, but it doesn't seem worse in any way. I can make that change if you want. I'm fine with it.

chore: update sprocket to latest WDL crate

b3dbcee

This also adds the ability to check/lint URLs

a-frantz requested a review from peterhuene December 17, 2024 22:51

a-frantz self-assigned this Dec 17, 2024

a-frantz added 2 commits December 18, 2024 10:16

feat: --local-only and --single-document args to check

d0d5b0a

Update CHANGELOG.md

4ebabcd

a-frantz marked this pull request as ready for review December 18, 2024 15:28

a-frantz requested review from adthrasher and claymcleod December 18, 2024 15:29

adthrasher reviewed Dec 18, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

a-frantz marked this pull request as draft December 18, 2024 16:31

a-frantz added 6 commits December 18, 2024 12:43

Update CHANGELOG.md

a35bf8b

chore: cargo update

6e1c196

chore: fmt

c1cbd2b

feat: add shellcheck

cab0c02

docs: label shellcheck experimental

b061ef1

Update Cargo.lock

2b81661

adthrasher reviewed Jan 2, 2025

View reviewed changes

a-frantz commented Jan 2, 2025

View reviewed changes

adthrasher reviewed Jan 2, 2025

View reviewed changes

a-frantz added 2 commits January 9, 2025 15:55

chore: update to latest

d2d7353

chore: clippy and dmt

faf1dbf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update sprocket to latest WDL crate #46

chore: update sprocket to latest WDL crate #46

a-frantz commented Dec 17, 2024 •

edited

Loading

a-frantz commented Dec 18, 2024

adthrasher Jan 2, 2025

a-frantz Jan 2, 2025

adthrasher Jan 2, 2025

adthrasher Jan 2, 2025

a-frantz Jan 2, 2025

adthrasher Jan 2, 2025

a-frantz Jan 2, 2025

adthrasher Jan 2, 2025

a-frantz Jan 2, 2025

adthrasher Jan 2, 2025

a-frantz Jan 2, 2025

adthrasher Jan 2, 2025

a-frantz Jan 2, 2025

adthrasher Jan 2, 2025

a-frantz Jan 2, 2025

adthrasher Jan 2, 2025

a-frantz Jan 2, 2025

	"failing due to {error_count} error{s}",
	"failing with {error_count} error{s}",

chore: update sprocket to latest WDL crate #46

Are you sure you want to change the base?

chore: update sprocket to latest WDL crate #46

Conversation

a-frantz commented Dec 17, 2024 • edited Loading

a-frantz commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-frantz commented Dec 17, 2024 •

edited

Loading