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

[FIX] JSModuleAnalyzer: Fix detection of bundle name #705

Merged
merged 4 commits into from
Apr 25, 2022

Conversation

matz3
Copy link
Member

@matz3 matz3 commented Feb 17, 2022

No description provided.

@matz3 matz3 force-pushed the fix-JSModuleAnalyzer branch from 2a9a916 to 57bcf31 Compare April 21, 2022 09:22
@coveralls
Copy link

coveralls commented Apr 21, 2022

Coverage Status

Coverage increased (+0.05%) to 94.887% when pulling 797ad6b on fix-JSModuleAnalyzer into 4493b13 on next.

@matz3 matz3 marked this pull request as ready for review April 21, 2022 11:36
@matz3 matz3 requested a review from codeworrior April 21, 2022 11:37
@matz3 matz3 changed the title [FIX] JSModuleAnalyzer: Fix handling of jQuery.sap.declare [FIX] JSModuleAnalyzer: Fix detection of bundle name Apr 21, 2022
@matz3
Copy link
Member Author

matz3 commented Apr 21, 2022

Code is ready for review. I will finish and enhance the tests soon.

@matz3
Copy link
Member Author

matz3 commented Apr 22, 2022

I will finish and enhance the tests soon.

Done

@RandomByte
Copy link
Member

LGTM

const info = analyzeString(content, "modules/module-with-jquery-sap-declare.js");
t.is(info.name, "modules/module-with-jquery-sap-declare.js");
t.is(info.rawModule, false);
t.is(info.format, "ui5-declare", "Format should be declare once a jQuery.sap.declare statement is found");
Copy link
Member

Choose a reason for hiding this comment

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

From the content I would say that the format should be ui5-define, not ui5-declare.

I understand that this was not changed by this PR (and rather is a sleeping bug in the analyzer). But seeing now a test that declares this the "expected" behavior, feels wrong.

Can we at least comment that this needs to be fixed? Or is there a 'todo' variant for 'test' (similar to QUnit.todo)?

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

@matz3 matz3 merged commit aaeafd4 into next Apr 25, 2022
@matz3 matz3 deleted the fix-JSModuleAnalyzer branch April 25, 2022 15:03
@matz3 matz3 added the bug Something isn't working label Apr 25, 2022
This was referenced May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants