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

Add hierarchical module IDs #191

Merged

Conversation

matejpavlovic
Copy link
Contributor

In preparation for submodules in Mir this commit introduces structured
module IDs. They are backwards-compatible with the old unstructured
ones, which are considered to be identifiers of top-level modules.

Copy link
Contributor

@xosmig xosmig left a comment

Choose a reason for hiding this comment

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

Thanks!

// It corresponds to a path in the module hierarchy.

In fact, it doesn't even have to be a path in the module hierarchy.
From the Mir core point of view, only the part of the module id until the first separator is used, the rest is just ignored by Mir.
It is up to the module implementations to decide what they do with the ignored part.

UPD: I am not asking you to change anything, just wanted to leave a note on this.

pkg/types/moduleid.go Outdated Show resolved Hide resolved
In preparation for submodules in Mir this commit introduces structured
module IDs. They are backwards-compatible with the old unstructured
ones, which are considered to be identifiers of top-level modules.

Signed-off-by: Matej Pavlovic <[email protected]>
@matejpavlovic
Copy link
Contributor Author

Thanks, good points, I still extended the comments, so the note doesn't get lost in an old pull request nobody looks at any more.

@matejpavlovic matejpavlovic merged commit 760efbe into consensus-shipyard:main Aug 18, 2022
@matejpavlovic matejpavlovic deleted the structured-module-ids branch August 18, 2022 15:14
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.

2 participants