Skip to content

Add file dependencies to the the output #61

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

Closed

Conversation

johnynek
Copy link

@johnynek johnynek commented Sep 1, 2018

closes #60

This is for suggestions and discussions on how to improve.

There are no new tests that parsing works (and I don't think it does). I will add those after we agree on the basic direction. The current Integration tests also fail since they are not expecting the file output.

Thanks for taking the time to review.

cc @SethTisue

@johnynek
Copy link
Author

johnynek commented Sep 5, 2018

@SethTisue any feedback on this approach?

@SethTisue
Copy link
Contributor

@johnynek I'm sorry I haven't managed to reply to you about this yet :-(

it's very near the top of my queue, I'm just a little overwhelmed right now with Scala 2.13.0-M5 related stuff.

@SethTisue
Copy link
Contributor

what do you think about a different approach where instead of adding new edges, we just change

case class Entity(name: String, kind: EntityKind)

to

case class Entity(name: String, kind: EntityKind, path: java.io.File)

then in Entity#forSymbol we'd change

    Entity(sym.nameString, kind)

to

    Entity(sym.nameString, kind, sym.associatedFile.canonicalPath)

that said, the design in this PR seems plausible to me also. so I'm just throwing this other possibility out there, but I'd consider the change mergeable regardless of which path you decide to go down.

@SethTisue SethTisue changed the title WIP: Add file depenendencies to the the output WIP: Add file dependencies to the the output Sep 10, 2018
@SethTisue SethTisue added the WIP label Feb 5, 2019
@johnynek
Copy link
Author

Hey @SethTisue I had a bit of energy so I was going to look at this again. Your suggestion breaks all the existing tests because now all entities have to have a path encoded. If anyone serialized any output, they would all be broken unless paths are optional, which makes additional complexity.

The next issue is that the graph of using probably shouldn't have the file information on each entity. Like sym: foo in filex "uses" sym: y in filez seems redundant since you can join the symbol information to see which file declared the symbol.

You probably have a lot more opinions about how to get this information out, maybe the easiest thing for me in the short run is just to use my patch and you can decide if/how you want to expose this information?

@SethTisue
Copy link
Contributor

(I'll have to think about this in early January.)

@SethTisue
Copy link
Contributor

SethTisue commented Feb 5, 2020

@johnynek yeah, I like your way, I'm convinced. happy to merge this if you can get CI to sign off.

@SethTisue SethTisue changed the title WIP: Add file dependencies to the the output Add file dependencies to the the output Oct 19, 2020
@SethTisue SethTisue removed the WIP label Oct 19, 2020
@SethTisue SethTisue marked this pull request as draft October 19, 2020 21:21
Base automatically changed from master to main March 19, 2021 19:31
@SethTisue SethTisue closed this May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

feature: adding an edge for file to symbol
3 participants