Improve handling of unknown QL pack roots for multi-query MRVAs #3289
Conversation
charisk
force-pushed
the
charisk/multi-query-ql-pack-roots
branch
from
377177a to
d2265db
Compare
charisk
force-pushed
the
charisk/multi-query-ql-pack-roots
branch
3 times, most recently
from
2c279db to
3306bc6
Compare
charisk
force-pushed
the
charisk/multi-query-ql-pack-roots
branch
from
3306bc6 to
96a3f31
Compare
There was a problem hiding this comment.
I like the behaviour this is aiming for, but I have quite a few comments about the specific implementation.
Apologies for being quite exacting about findCommonParentDir. Since we're writing a shared helper file I'd like if we make sure it's as correct as we can make it. I recognise we might not hit these edge-cases right now, but it'll help avoid hard-to-detect bugs if someone else starts using this function in the future.
| packRoots.push(packRoot); | ||
| } | ||
|
|
||
| if (queryFiles.length === 1) { |
There was a problem hiding this comment.
Do we need this special case for one query? I think the logic below still works when there's a single file. Since there's not much below this I wonder if it's simpler to reduce the number of special cases, rather returning early and reducing the amount of code executed.
There was a problem hiding this comment.
I think there is a slight difference when there is a single query. If the query is not in the workspace, we currently allow MRVA to run. So I wanted to keep that behaviour. For multiple queries it gets more complicated, because if they're not in a pack we want to find a common dir for them so they need to be in the workspace to avoid reaching places in the filesystem where we shouldn't.
There was a problem hiding this comment.
If the query is not in the workspace, we currently allow MRVA to run. So I wanted to keep that behaviour.
Why do we want this behaviour? To me it seems like an oversight and unlikely that anyone would be relying on this to run MRVA using query files outside of their workspace.
Would it not be better to have consistent behaviour when there's one query vs multiple queries?
There was a problem hiding this comment.
I could have opened a workspace and dragged a query file from somewhere else into VS Code. So I can imagine that happening, but definitely not often.
I agree it would be good to have the same behaviour but perhaps if we want to change it we should have a specific PR for that, with a CHANGELOG note. WDYT?
There was a problem hiding this comment.
A separate PR with a changelog note sounds good to me. Also getting eyes from more members of the team on whether this makes sense. I don't really have any strong opinions.
There was a problem hiding this comment.
Code looks good to me. If we want to make any more tweaks I think they can be separate PRs. Thankfully everything is tested well so we should be covered if we do change the methods.
I've also briefly tried it out in a workspace with no query packs and it appeared to work just fine.
Add some logic to handle calculating QL pack roots for MRVA better.
Checklist
N/A:
ready-for-doc-reviewlabel there.