feat(jsii): record source locations in assembly #429
Conversation
packages/jsii-reflect/lib/method.ts
Outdated
| /** | ||
| * Return the location in the module | ||
| */ | ||
| public get moduleLocation(): SourceLocation | undefined { |
There was a problem hiding this comment.
The name suggests it'll return the location of the module, not in the module. I don't really like how this is named moduleLocation anyway, I'd rather have this named sourceLocation, really.
There was a problem hiding this comment.
I originally had it as sourceLocation, but I want the name to be clear about what it's relative to. In this case, it's a module-relative location.
It becomes relevant later when we're combining it with the assembly path to get a repository-relative location.
There was a problem hiding this comment.
I still think moduleLocation is misleading, especially considering your argument.
You could name that sourceCoordinates (to me, "coordinates" feels more "relative to some referential") than "location")
packages/jsii-reflect/lib/source.ts
Outdated
|
|
||
| interface Locatable { | ||
| readonly assembly: Assembly; | ||
| readonly moduleLocation?: SourceLocation; |
There was a problem hiding this comment.
Would name soureLocation, or maybe even just location.
| @@ -28,6 +28,7 @@ | |||
| "jest": "^24.1.0", | |||
| "jsii-build-tools": "^0.8.2", | |||
| "jsii-calc": "^0.8.2", | |||
| "jsii": "^0.8.2", | |||
| "ts-jest": "^24.0.0", | |||
| "typescript": "^3.3.3333" | |||
There was a problem hiding this comment.
If you depend on jsii, you shouldn't depend on typescript anymore...
There was a problem hiding this comment.
Strongly disagree. Just because dependencies exist somewhere down the dependency tree, doesn't mean I should just invisibly use them. You should always declare your direct dependencies, it's the only way to do proper dependency management.
If we were to rewrite jsii to Java, jsii-reflect would break for no good reason.
If we were to need to depend on a different version of typescript between these 2 packages, jsii-reflect would break for no good reason.
There was a problem hiding this comment.
Why do you depend on jsii then? If it's not used instead of tic, then I don't think it should be depended on at all...
There was a problem hiding this comment.
We depend on jsii because the tests depend on jsii. They do a "compile this source and then check that the reflection of its assembly has these properties" test.
packages/jsii-spec/lib/spec.ts
Outdated
| * available. If the module root is equal to the root of the repository, | ||
| * the value is '.'. | ||
| */ | ||
| repositoryLocation?: string; |
There was a problem hiding this comment.
repositoryPath? pathInRepository?
packages/jsii-spec/lib/spec.ts
Outdated
| /** | ||
| * Where in the module this definition was found | ||
| */ | ||
| moduleLocation?: SourceLocation; |
There was a problem hiding this comment.
sourceLocation?: SourceLocation.
Also, since this is present on a number of things, why not introduce a SourceLocatable interface and extend that?
There was a problem hiding this comment.
I guess. Doesn't seem so valuable but I can do that.
There was a problem hiding this comment.
Well for one, if you want to rename those... There's only one place... And it allows stuff that uses that to be less tied to the list of specific types that have the sourceLocation property.
packages/jsii/lib/assembler.ts
Outdated
| const file = node.getSourceFile(); | ||
| const line = ts.getLineAndCharacterOfPosition(file, node.getStart()).line; | ||
| return { | ||
| filename: path.relative(this.projectInfo.projectRoot, path.resolve(file.fileName)), |
There was a problem hiding this comment.
Is the path.resolve necessary here?
There was a problem hiding this comment.
I don't know. I added it for safety. Guess I should put one around projectRoot too.
There was a problem hiding this comment.
Was asking mostly for curiosity :)
There was a problem hiding this comment.
I tried it out and it's actually not necessary!
| @@ -38,6 +39,7 @@ export = { | |||
| fqn: 'testpkg.Foo', | |||
| kind: 'enum', | |||
| members: [{ name: 'Bar' }, { name: 'Baz' }], | |||
There was a problem hiding this comment.
Would be nice if the members were also locatable.... 😇 (But ... like not important at all).
| @@ -364,6 +444,10 @@ | |||
| "summary": "The value." | |||
| }, | |||
| "immutable": true, | |||
| "locationInModule": { | |||
There was a problem hiding this comment.
Wouldn't it be sufficient to use a single line with : to separate file/line? Just thinking of how much global energy this will preserve.
There was a problem hiding this comment.
Aren't you the king of Worse Is Better ;)
There was a problem hiding this comment.
Always. That's what I strive for. For example, stringified tokens.
| /** | ||
| * Return the location in the repository | ||
| */ | ||
| public get locationInRepository(): SourceLocation | undefined { |
There was a problem hiding this comment.
maybe a base class for Property/Method that reuses docs/location and other stuff
There was a problem hiding this comment.
Probably a good idea. The next person after me can do it, I already made one for ClassType/InterfaceType.
| @@ -28,6 +28,7 @@ | |||
| "jest": "^24.1.0", | |||
| "jsii-build-tools": "^0.8.2", | |||
| "jsii-calc": "^0.8.2", | |||
| "jsii": "^0.8.2", | |||
| "ts-jest": "^24.0.0", | |||
| "typescript": "^3.3.3333" | |||
Add the source locations of symbols to the JSII assembly, and give jsii-reflect some routines to query those, so that we'll be able to generate source location hyperlinks in the API reference.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.