feat: Nice errors for shim flag parsing #6737
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
8 Ignored Deployments
|
🟢 Turbopack Benchmark CI successful 🟢Thanks |
🟢 CI successful 🟢Thanks |
Linux Benchmark for 22ea3eeClick to view benchmark
|
NicholasLYang
force-pushed
the
nicholasyang/error-provenance
branch
from
275fdaa
to
565e725
Compare
Linux Benchmark for 13c90b5Click to view benchmark
|
NicholasLYang
force-pushed
the
nicholasyang/error-provenance
branch
from
151e0be
to
adb8c44
Compare
NicholasLYang
force-pushed
the
nicholasyang/error-provenance
branch
from
adb8c44
to
38f8ad5
Compare
NicholasLYang
force-pushed
the
nicholasyang/error-provenance
branch
from
38f8ad5
to
d00ee50
Compare
NicholasLYang
force-pushed
the
nicholasyang/error-provenance
branch
from
d00ee50
to
d48f895
Compare
NicholasLYang
changed the title
Linux Benchmark for 65d4d8eClick to view benchmark
|
Linux Benchmark for 02e3c1aClick to view benchmark
|
NicholasLYang
force-pushed
the
nicholasyang/error-provenance
branch
from
682368b
to
941ab9a
Compare
Linux Benchmark for 7c45361Click to view benchmark
|
Linux Benchmark for b1c68b7Click to view benchmark
|
chris-olszewski
approved these changes
Dec 12, 2023
| flag2: Option<SourceSpan>, | ||
| #[label("and here")] | ||
| flag3: Option<SourceSpan>, | ||
| // The user should get the idea after the first 4 examples |
There was a problem hiding this comment.
Haha, I would hope so. I would hope 2 examples would be enough.
| ) -> Vec<usize> { | ||
| // Sort the indices to keep the invariant | ||
| // that if i > j then output[i] > output[j] | ||
| args_indices.sort(); |
There was a problem hiding this comment.
Do we need to require callers to keep up this invariant? e.g. If I pass in vec![2, 1] and get out vec![3, 6] would I be in a bad state when I try to map the indices to the args.
There was a problem hiding this comment.
Well I think the idea is you shouldn't be mapping these to args, since they're now indices into the string. And unless you parse the string it's not really easy to map the indices to the args.
crates/turborepo-lib/src/shim.rs
Outdated
Comment on lines
232
to
235
| pub(crate) fn get_indices_in_args_string( | ||
| mut args_indices: Vec<usize>, | ||
| args: impl Iterator<Item = impl Into<String>>, | ||
| ) -> Vec<usize> { |
There was a problem hiding this comment.
This feels like a "sharp" interface and I have a few suggestions:
- Make this a private function
- Since this is only used for making pretty error messages, I don't think we need to make this as perf sensitive as it currently is. We can generate the indices into the arg strings for all args and not just ones passed in as
arg_indices. - Based on how we're using this. Do we just want to calculate the spans here instead of having callers do stuff like
(idx, "--flag".len())?
Co-authored-by: Chris Olszewski <chris.olszewski@vercel.com>
kdy1
added a commit
to vercel/next.js
that referenced
this pull request
Jan 2, 2024
### What? Update SWC crates ### Why? It's required to fix some next.js isssues ### How? Closes PACK-2174 Turbopack counterpart: vercel/turborepo#6843 --- # Turbopack * vercel/turborepo#6737 <!-- Nicholas Yang - feat: Nice errors for shim flag parsing --> * vercel/turborepo#6494 <!-- Justin Ridgewell - Trace output's source maps through input's source map --> * vercel/turborepo#6778 <!-- Will Binns-Smith - turbopack-css: fix rule duplication --> * vercel/turborepo#6807 <!-- Mehul Kar - fix: add missing dependency to cargo lockfile --> * vercel/turborepo#6809 <!-- Nicholas Yang - refactor: Remove bookkeeping abstraction from turbo config --> * vercel/turborepo#6838 <!-- Nicholas Yang - fix: Allow for long symlinks by using append_link --> * vercel/turborepo#6845 <!-- Donny/강동윤 - perf: Use `&'static str` instead of `String` if possible --> * vercel/turborepo#6843 <!-- Donny/강동윤 - Update `swc_core` to `v0.87.10` -->
agustints
pushed a commit
to agustints/next.js
that referenced
this pull request
Jan 6, 2024
### What? Update SWC crates ### Why? It's required to fix some next.js isssues ### How? Closes PACK-2174 Turbopack counterpart: vercel/turborepo#6843 --- # Turbopack * vercel/turborepo#6737 <!-- Nicholas Yang - feat: Nice errors for shim flag parsing --> * vercel/turborepo#6494 <!-- Justin Ridgewell - Trace output's source maps through input's source map --> * vercel/turborepo#6778 <!-- Will Binns-Smith - turbopack-css: fix rule duplication --> * vercel/turborepo#6807 <!-- Mehul Kar - fix: add missing dependency to cargo lockfile --> * vercel/turborepo#6809 <!-- Nicholas Yang - refactor: Remove bookkeeping abstraction from turbo config --> * vercel/turborepo#6838 <!-- Nicholas Yang - fix: Allow for long symlinks by using append_link --> * vercel/turborepo#6845 <!-- Donny/강동윤 - perf: Use `&'static str` instead of `String` if possible --> * vercel/turborepo#6843 <!-- Donny/강동윤 - Update `swc_core` to `v0.87.10` -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This adds nice errors for shim parsing:
Testing Instructions
Added test for error output
Closes TURBO-1874