Abstract project names #12106
Abstract project names #12106
Conversation
kazcw
requested review from
Frizi,
farmaazon,
vitvakatu and
AdRiley
as code owners
🧪 Storybook is successfully deployed!📊 Dashboard:
|
|
|
||
| normalized(): ProjectPath { | ||
| if (!this.path) return this | ||
| const normalized = this.path.match(/^Main(\\..+)?$/) |
There was a problem hiding this comment.
Gratuitous \ - it matches Main\ab but not Main.ab
Add test/case for this method
| /** `undefined` identifies the current project; otherwise, this will be a two-segment path. */ | ||
| readonly project: QualifiedName | undefined, |
There was a problem hiding this comment.
I would add a guard in this class to be more explicit what undefined means.
For example
notCurrentProject(): this is AbsoluteProjectPath {
return this.project != null
}(I would like to reverse it to isCurrentProject, but I don't know how to keep the type guard).
There was a problem hiding this comment.
I agree that the meaning of undefined is not self-evident here, but I think it's better not to have a predicate for AbsoluteProjectPath. Really the only reason AbsoluteProjectPath exists is so that when a (non current-project) ProjectPath constant is defined, the corresponding QualifiedName can be obtained without needing a ProjectNameStore instance. Testing whether a path is absolute at runtime would not be useful because if it isn't, a projectNames instance will be needed anyway, and so it's best just to handle the values uniformly using the general functions from projectNames.
| private readonly pathToId = new ReactiveIndex(this, (id, entry) => [ | ||
| [pathKey(entry.definitionPath), id], | ||
| ]) |
There was a problem hiding this comment.
Is private necessary here?
There was a problem hiding this comment.
It is not useful without pathKey, which is an implementation detail
| function parseProjectPath(path: QualifiedName): ProjectPath { | ||
| const parsed = parseAbsoluteProjectPath(path) | ||
| return parsed.project === inboundProject.value ? | ||
| ProjectPath.create(undefined, parsed.path) | ||
| : parsed | ||
| } | ||
|
|
||
| function printProjectPath(path: ProjectPath): QualifiedName { | ||
| return normalizeQualifiedName(printProjectPathDenormalized(path)) | ||
| } | ||
|
|
||
| function printProjectPathDenormalized(path: ProjectPath): QualifiedName { | ||
| const project = path.project ?? outboundProject.value | ||
| return path.path ? qnJoin(project, path.path) : project | ||
| } |
There was a problem hiding this comment.
It would be nice to have tests for those functions.
| imported: ImportedNames | ||
| } | ||
|
|
||
| export type RawImport = Import<QualifiedName> | ||
| export type AbstractImport = Import<ProjectPath> |
There was a problem hiding this comment.
Why is this called Abstract? Neither it misses some information, nor I see any specializations
There was a problem hiding this comment.
ProjectPath abstracts the name of the current project
Pull Request Description
Use abstract project references in retained state.
Fixes #11815.
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.