Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(jsii): record source locations in assembly #429

Merged
merged 6 commits into from
Apr 4, 2019

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Apr 4, 2019

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.

@rix0rrr rix0rrr requested a review from a team as a code owner April 4, 2019 07:52
.vscode/tasks.json Outdated Show resolved Hide resolved
/**
* Return the location in the module
*/
public get moduleLocation(): SourceLocation | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")


interface Locatable {
readonly assembly: Assembly;
readonly moduleLocation?: SourceLocation;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would name soureLocation, or maybe even just location.

packages/jsii-reflect/lib/source.ts Outdated Show resolved Hide resolved
packages/jsii-reflect/lib/source.ts Show resolved Hide resolved
@@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you depend on jsii, you shouldn't depend on typescript anymore...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

@rix0rrr rix0rrr Apr 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am with Rico.

* available. If the module root is equal to the root of the repository,
* the value is '.'.
*/
repositoryLocation?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

repositoryPath? pathInRepository?

/**
* Where in the module this definition was found
*/
moduleLocation?: SourceLocation;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sourceLocation?: SourceLocation.

Also, since this is present on a number of things, why not introduce a SourceLocatable interface and extend that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess. Doesn't seem so valuable but I can do that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const file = node.getSourceFile();
const line = ts.getLineAndCharacterOfPosition(file, node.getStart()).line;
return {
filename: path.relative(this.projectInfo.projectRoot, path.resolve(file.fileName)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the path.resolve necessary here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. I added it for safety. Guess I should put one around projectRoot too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was asking mostly for curiosity :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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' }],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't you the king of Worse Is Better ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always. That's what I strive for. For example, stringified tokens.

/**
* Return the location in the repository
*/
public get locationInRepository(): SourceLocation | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a base class for Property/Method that reuses docs/location and other stuff

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am with Rico.

@rix0rrr rix0rrr merged commit e601c0c into master Apr 4, 2019
@rix0rrr rix0rrr deleted the huijbers/source-code-location branch April 4, 2019 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants