Fix handling of packages in damlc visual #5789
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously we just ran the analysis on the modules of the main package. This failed for obvious reasons as soon as you reference a template from another package which happens pretty frequently (e.g. for anything that uses finlib). This PR fixes this to run the analysis on the whole World which is self-contained. This required a bunch of reshuffling to make sure that we always reference fully qualified identifiers but most of it is very mechanical. Note that currently you cannot distinguish between templates with identical names in the resulting graph (they will be separate but you have no idea which one is which). This was already an issue before if you have the same template name in different modules so I consider this an orthogonal issue. This fixes the expected failure we already had and I added another test that checks that colliding template names do at least show up as separate nodes in the graph. I also manually tested this against ex-bond-issuance. Disclaimier: I’m aware that the code is very messy but I tried to resist the urge to rewrite it completely and only change what was necessary. fixes #5776 changelog_begin - [DAML Compiler] ``damlc visual`` now works properly in projects consisting of multiple packages. changelog_end
hurryabit
approved these changes
May 4, 2020
| , choiceAndActions :: [ChoiceAndAction] | ||
| } deriving (Show) | ||
|
|
||
| templateChoiceId :: TemplateChoices -> LF.Qualified LF.TypeConName |
There was a problem hiding this comment.
I find this function name misleading. I understand that it returns a "choice id" when it actually returns a "template id". Maybe something like templateIdFromChoices would be better.
There was a problem hiding this comment.
Good point, I’ve renamed it to just templateId.
changelog_begin changelog_end
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously we just ran the analysis on the modules of the main
package. This failed for obvious reasons as soon as you reference a
template from another package which happens pretty
frequently (e.g. for anything that uses finlib).
This PR fixes this to run the analysis on the whole World which is
self-contained. This required a bunch of reshuffling to make sure that
we always reference fully qualified identifiers but most of it is
very mechanical.
Note that currently you cannot distinguish between templates with
identical names in the resulting graph (they will be separate but you
have no idea which one is which). This was already an issue
before if you have the same template name in different modules so I
consider this an orthogonal issue.
This fixes the expected failure we already had and I added another
test that checks that colliding template names do at least show up as
separate nodes in the graph. I also manually tested this against
ex-bond-issuance.
Disclaimier: I’m aware that the code is very messy but I tried to
resist the urge to rewrite it completely and only change what was
necessary.
fixes #5776
changelog_begin
damlc visualnow works properly in projectsconsisting of multiple packages.
changelog_end
Pull Request Checklist
CHANGELOG_BEGINandCHANGELOG_ENDtagsNOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with
/AzurePipelines runtotrigger the build.