Force directory param to be required on API calls and refactor config-based watchers to work in subdirectories #1953
Conversation
…ers for package files to work with subdirectories, and updated package views to work in subdirectories. Updated Bus callbacks to be typesafe.
sagerb
requested review from
dotNomad,
kgartland-rstudio,
mmarchetti and
tdstein
as code owners
extensions/vscode/src/watchers.ts
Outdated
| contentRecord.projectDir, | ||
| ".posit", | ||
| "publisher", | ||
| contentRecord.configurationName, |
There was a problem hiding this comment.
Can you clarify why this changed from cfg.configurationPath? Also, does it need a .toml suffix?
There was a problem hiding this comment.
Yeah I'm a bit confused by this as well. We have the configurationPath why not just use that as before with new RelativePattern and without making our own path?
There was a problem hiding this comment.
I'll check, but I think this was probably just an oversight. I'll mark this and the other as being switched back and will test it from there. I'm sure the both of you are probably right.
| get(configName: string, params?: { dir?: string }) { | ||
| get(configName: string, params: { dir: string }) { |
There was a problem hiding this comment.
I understand this is to avoid us from forgetting the dir param when we should include it, but I do think we should discuss the value we are losing here. With this change the api module diverges from representing the API.
Personally I would prefer the module be a 1-1 with the actual API functionality. Mainly because I believe it causes confusion for anyone who isn't intimately familiar with the patterns of this code base why a URL param (which is normally optional) is not optional here.
| params: { dir: string; entrypoint?: string; recursive?: boolean }, | ||
| python?: string, | ||
| params?: { dir?: string; entrypoint?: string; recursive?: boolean }, |
There was a problem hiding this comment.
I found this switch very confusing. The Axios functions, and all of our others do data or parameters to fill the data/URL, and then params. Seeing the function change elsewhere really got me scratching my head before I came back here to discover the movement.
There was a problem hiding this comment.
I did not like it either, but it comes at the language requirement to maintain the order of required then optional parameters. I'm good with refactoring this to avoid this awkwardness, but I do want to maintain the typescript parameter checks as they were invaluable to the refactoring work I was doing and I believe to what we have yet to do in the future.
extensions/vscode/src/watchers.ts
Outdated
| contentRecord.projectDir, | ||
| ".posit", | ||
| "publisher", | ||
| contentRecord.configurationName, |
There was a problem hiding this comment.
Yeah I'm a bit confused by this as well. We have the configurationPath why not just use that as before with new RelativePattern and without making our own path?
refactored changes to make dir mandatory for API calls. Updated watchers for package files to work with subdirectories, and updated package views to work in subdirectories. Updated Bus callbacks to be typesafe.
Intent
Refactored changes to make dir mandatory for API calls.
Updated watchers for package files to work with subdirectories
Updated package views to work in subdirectories.
Updated Bus callbacks to be typesafe.
Resolves #1855
Resolves #1949
Type of Change
Directions for Reviewers
Use VSCode to open project's subdirectory directly as well as from a few parent subdirectories up.
With these projects, verify the functionality related to packages for both R and Python projects.
Be sure to both manually and automatically update the package file, along with verifying that a non-default package file name within the active config file will be watched and update within the view.