-
Notifications
You must be signed in to change notification settings - Fork 39
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
Revise launch config #2347
Revise launch config #2347
Conversation
🦋 Changeset detectedLatest commit: 2732943 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes looks good to me, but it has a potential for additional refactoring as some functions looks more or less the same as already existing ones.
Also no issues with tests, tested locally on Windows.
@broksy @Klaus-Keller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did not get to go through all changes, but saw the OData version type is not being reused. Please adjust this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor things from previous review are fixed.
Thanks, @kjose90!
Approved from my side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kjose90,
Please see some linting comments. Otherwise, code looks good.
…-tools into revise-launch-config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kjose90!
- changes and revise launch config looks good
- change set exists
- code coverage is good
- did a visual review, no manual tests
Approved from my side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-approved after fixed linting issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @kjose90 changes look good.
One comment I noticed while reviewing, but not directly related to this PR.
Thanks everyone for reviewing |
Quality Gate passedIssues Measures |
* main: chore: apply latest changesets feat(fpm-writer): property `allowAutoAddDependencyLib` to the building block API, which allows turning off the automatic addition of the 'sap.fe.macros' library (#2393) chore: apply latest changesets fix: missing unique react key (#2368) chore: apply latest changesets fix: theme picker hover and border for dropdowns (#2377) chore: apply latest changesets fix(preview-middleware): manifest changes not visible after reload (#2385) chore: apply latest changesets fix(ui-c): accessibility updates for uitable (#2391) chore: apply latest changesets Revise launch config (#2347) chore: apply latest changesets feat: app info file handling logic to new module @sap-ux/fiori-tools-settings (#2379)
@@ -60,22 +60,30 @@ export interface LaunchConfigInfo { | |||
filePath: string; | |||
} | |||
|
|||
/** | |||
* Enum representing the types of data sources or origins for a project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain what impact each has on the launch config
mem-fs
instead to write launch config fileswriteApplicationInfoSettings()
from@sap-ux/launch-config
createLaunchConfig