Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: rename lightning component bundles #3923
feat: rename lightning component bundles #3923
Changes from 15 commits
a415e55
c441da1
6b31049
00f58f9
e77dc20
9946fd4
a73e931
eca0e57
7677870
a6ca470
8033b28
c373a98
2a666f4
8c1144e
ae399f9
1b616ae
c140c0a
8c9003d
7759f04
14100bf
7a1c779
3329033
8027eae
1bc9904
fa6d72d
5235c6f
64d4416
b18b4fa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
nit: you can combine these into a single if statement
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.
Should we add a log or telemetry for when we encounter the case where either of these values is falsy?
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.
@gbockus-sf Does these conditions are already includes into telemetry since this class extends LibraryCommandletExecutor?
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.
swinging back around to the telemetry. We do get a ping about this command being called b/c of the LibraryCommandletExecutor extends...however we don't know if we actually do it as determined by this logic. It's fine to push that off for now as we need a plan, but I was just calling it out previously.
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.
I still think everything below here should probably be in a service/util class...but get have have existing patterns we should break away from.
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.
@gbockus-sf This can be Dev Choice problem that we can discuss within the team later, yes, I followed the existing pattern.
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.
This smells like a service method vs something that should live in the command. The command code should only really concern itself with command execution. The check for existing and actual renaming should live elsewhere. Maybe under a services directory in core or utils. It looks like we have some one off services here and there but nothing organizing the logic. @jag-sfdc any thoughts or collections of services that I'm unaware of in the codebase?
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.
dumb q: why is the count 5 here? Might be worth a code comment to call out what things we expect to be renamed if we are going to verify the call arguments.
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.
@gbockus-sf The previous test focus on the call arguments, this test is intended to make sure it only rename the files and folder that have same name with LWC component. Here in this lwc component directory, it has 6 files and only 4 files match the component name, so rename should be called 5 times (plus renaming the folder rename).