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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[FIX] versionInfoGenerator: manifest without embeds #486 #576

Merged
merged 2 commits into from
Jan 28, 2021
Merged

[FIX] versionInfoGenerator: manifest without embeds #486 #576

merged 2 commits into from
Jan 28, 2021

Conversation

DerGuteWolf
Copy link
Contributor

@DerGuteWolf DerGuteWolf commented Jan 27, 2021 •
edited by tobiasso85

manifestInfo.embeds can be initial and calling map on initial value will not work, thus default to empty array when initial

Thank you for your contribution! 馃檶

To get it merged faster, kindly review the checklist below:

Pull Request Checklist

@DerGuteWolf
Fix SAP#486
fe97a09 
manifestInfo.embeds can be initial and calling map on initial value will not work, thus default to empty array when initial
@CLAassistant
Copy link

CLAassistant commented Jan 27, 2021 •
edited

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Jan 27, 2021 •
edited

Coverage Status

Coverage increased (+0.002%) to 93.837% when pulling 715ff2a on DerGuteWolf:patch-1 into 390587b on SAP:master.

@DerGuteWolf DerGuteWolf mentioned this pull request Jan 27, 2021
Closed
tobiasso85
tobiasso85 suggested changes Jan 28, 2021
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.

Really good catch and a good fix, please also provide a test for this scenario
see similar test
https://github.com/SAP/ui5-builder/blob/master/test/lib/processors/versionInfoGenerator.js#L224

@tobiasso85 tobiasso85 added the bug Something isn't working label Jan 28, 2021
@DerGuteWolf
Copy link
Contributor Author

I added a similar test .

tobiasso85
tobiasso85 approved these changes Jan 28, 2021
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

@codeworrior
Copy link
Member

When merging, please adapt the summary line of the commit message according to our guidelines.

@DerGuteWolf
Copy link
Contributor Author

DerGuteWolf commented Jan 28, 2021 •
edited

Suggestion for commit message:

[FIX] VersionInfoGenerator Processor: Correct Handling of non-existing manifestInfo.embeds

  • manifestInfo.embeds can be initial when the sap.app section of a manifest does not contain an embeds property
  • thus default to empty array for further processing eg with map

Fixes: #486

@tobiasso85 tobiasso85 changed the title Fix #486 [FIX] versionInfoGenerator: manifest without embeds #486 Jan 28, 2021
@tobiasso85 tobiasso85 merged commit 4d86226 into SAP:master Jan 28, 2021
@tobiasso85
Copy link
Contributor

Fixes: SAP/ui5-tooling#486

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobiasso85 tobiasso85 tobiasso85 approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@DerGuteWolf @CLAassistant @coveralls @codeworrior @tobiasso85