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] Implement Resource Tagging Mechanism #480

Merged
merged 18 commits into from
Aug 11, 2020
Merged

[FEATURE] Implement Resource Tagging Mechanism #480

merged 18 commits into from
Aug 11, 2020

Conversation

RandomByte
Copy link
Member

@RandomByte RandomByte commented Jul 20, 2020 •
edited
Loading

@RandomByte RandomByte changed the title [INTERNAL] Add tests for BuildContext and ProjectBuildContext Implement Resource Tagging Mechanisms Jul 20, 2020
@RandomByte RandomByte force-pushed the resource-tagging branch 3 times, most recently from 72d7a6e to 10874e4 Compare July 23, 2020 14:36
@RandomByte RandomByte changed the title Implement Resource Tagging Mechanisms [FEATURE] Implement Resource Tagging Mechanisms Jul 23, 2020
@RandomByte RandomByte changed the title [FEATURE] Implement Resource Tagging Mechanisms [FEATURE] Implement Resource Tagging Mechanism Jul 23, 2020
@RandomByte RandomByte marked this pull request as ready for review July 23, 2020 16:56
@RandomByte RandomByte requested a review from matz3 July 23, 2020 16:56
@RandomByte
Copy link
Member Author

Depends on SAP/ui5-fs#261

@matz3 matz3 force-pushed the resource-tagging branch 3 times, most recently from 2fc0104 to fe814c6 Compare July 30, 2020 16:00
@coveralls
Copy link

coveralls commented Jul 30, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 91.591% when pulling 3fa2e70 on resource-tagging into 2f50261 on master.

matz3
matz3 previously approved these changes Jul 31, 2020
View reviewed changes
Lonwyr
Lonwyr previously approved these changes Aug 3, 2020
View reviewed changes
Copy link
Contributor

@Lonwyr Lonwyr left a comment

Choose a reason for hiding this comment

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

Had a look at the coding and checked the integration test results.
Works like a charm and also brings up the correct results.

RandomByte
RandomByte commented Aug 3, 2020
View reviewed changes
lib/builder/ProjectBuildContext.js
const ResourceTagCollection = require("@ui5/fs").ResourceTagCollection;

const STANDARD_TAGS = Object.freeze({
OmitFromBuildResult: "ui5:OmitFromBuildResult"
Copy link
Member Author

Choose a reason for hiding this comment

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

The only thing left to agree on: Naming of this tag

@matz3 and I went on to rename it from HideFromBuildResult to OmitFromBuildResult after the last planning meeting. Maybe there are still other opinions.

CC: @ecker

@RandomByte RandomByte dismissed stale reviews from Lonwyr and matz3 via 908f4f6 August 3, 2020 16:13
@RandomByte RandomByte requested a review from KlattG August 3, 2020 16:23
KlattG
KlattG suggested changes Aug 5, 2020
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.

Just a few remarks from my side.

lib/tasks/TaskUtil.js Outdated
* Convenience functions for UI5 Builder tasks.
* An instance of this class is passed to every standard UI5 Builder task that requires it.
*
* Custom tasks that define a Specification Version >= 2.2 will receive an interface
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
* Custom tasks that define a Specification Version >= 2.2 will receive an interface
* Custom tasks that define a specification version >= 2.2 will receive an interface

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

lib/tasks/TaskUtil.js Outdated Show resolved Hide resolved
lib/tasks/TaskUtil.js Outdated Show resolved Hide resolved
lib/tasks/TaskUtil.js Outdated Show resolved Hide resolved
lib/tasks/TaskUtil.js Outdated Show resolved Hide resolved
lib/tasks/TaskUtil.js Outdated Show resolved Hide resolved
lib/tasks/TaskUtil.js Outdated Show resolved Hide resolved
lib/tasks/TaskUtil.js Outdated Show resolved Hide resolved
lib/tasks/TaskUtil.js Show resolved Hide resolved
test/lib/types/library/libraryType.js Show resolved Hide resolved
@RandomByte RandomByte mentioned this pull request Aug 7, 2020
Merged
@tobiasso85 tobiasso85 self-requested a review August 11, 2020 07:54
RandomByte and others added 10 commits August 11, 2020 10:32
tobiasso85
tobiasso85 approved these changes Aug 11, 2020
View reviewed changes
Copy link
Contributor

@tobiasso85 tobiasso85 left a comment

Choose a reason for hiding this comment

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

LGTM

@RandomByte RandomByte merged commit c615e19 into master Aug 11, 2020
@RandomByte RandomByte deleted the resource-tagging branch August 11, 2020 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KlattG KlattG KlattG requested changes

@tobiasso85 tobiasso85 tobiasso85 approved these changes

@Lonwyr Lonwyr Lonwyr left review comments

@matz3 matz3 matz3 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@RandomByte @coveralls @matz3 @Lonwyr @tobiasso85 @KlattG