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.

Sign up for GitHub

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

Jump to bottom

[FEATURE] Workspace: Add getModules method / Specification: Add getId method #592

Merged
merged 3 commits into from
Mar 30, 2023
Merged

[FEATURE] Workspace: Add getModules method / Specification: Add getId method #592

merged 3 commits into from
Mar 30, 2023

Conversation

matz3
Copy link
Member

@matz3 matz3 commented Mar 29, 2023

Required for #589

matz3 added 2 commits March 29, 2023 13:51
@matz3
[FEATURE] Workspace: Add getModules method
46f95b2 
Can be used to retrieve all modules referenced in a workspace to
iterate over them.

Required for #589
@matz3
[FEATURE] Specification: Add getId method
e1c83b3 
Returns the ID defined by the provider. Should not be used for general
purposes for which "getName" should still be preferred.

Required for #589
@matz3 matz3 added the enhancement New feature or request label Mar 29, 2023
@matz3 matz3 requested review from a team and KlattG March 29, 2023 11:52
@coveralls
Copy link

coveralls commented Mar 29, 2023 •
edited
Loading

Coverage Status

Coverage: 95.578% (+0.006%) from 95.572% when pulling 5dd5cb0 on add-new-apis into 2b41f2a on main.

flovogt
flovogt reviewed Mar 29, 2023
View reviewed changes
Copy link
Member

@flovogt flovogt left a comment

Choose a reason for hiding this comment

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

LGTM. Waiting for review by @KlattG

KlattG
KlattG suggested changes Mar 30, 2023
View reviewed changes
lib/graph/Workspace.js Outdated Show resolved Hide resolved
lib/graph/Workspace.js Outdated Show resolved Hide resolved
lib/specifications/Specification.js Outdated Show resolved Hide resolved
lib/specifications/Specification.js Outdated Show resolved Hide resolved
lib/specifications/Specification.js Outdated Show resolved Hide resolved
@matz3 matz3 requested a review from KlattG March 30, 2023 12:11
KlattG
KlattG suggested changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@KlattG KlattG left a comment

Choose a reason for hiding this comment

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

Typo

lib/specifications/Specification.js Outdated
@@ -177,7 +195,7 @@ class Specification {
}

/**
* Get the type of this specification,
* Gest the type of this specification,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Gest the type of this specification,
* Gets the type of this specification,

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. Thanks.

@matz3 @KlattG
[INTERNAL] Apply suggestions from code review
5dd5cb0 
Co-authored-by: Günter Klatt <57760635+KlattG@users.noreply.github.com>
KlattG
KlattG approved these changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@KlattG KlattG left a comment

Choose a reason for hiding this comment

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

LGTM

@matz3 matz3 merged commit 7ee6dde into main Mar 30, 2023
@matz3 matz3 deleted the add-new-apis branch March 30, 2023 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flovogt flovogt flovogt left review comments

@KlattG KlattG KlattG approved these changes

@d3xter666 d3xter666 d3xter666 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@matz3 @coveralls @d3xter666 @flovogt @KlattG