@W-9729358@ Make CPD engine production ready #522
Conversation
| export const CpdRuleCategory = 'Copy/Paste Detected'; | ||
| export const CpdInfoUrl = 'https://pmd.github.io/latest/pmd_userdocs_cpd.html#refactoring-duplicates'; | ||
| export const CpdViolationSeverity = Severity.LOW; | ||
| export const CpdLanguagesSupported: LANGUAGE[] = [...new Set (FileExtToLanguage.values())]; |
There was a problem hiding this comment.
As discussed, languages supported will be deduced from the file extensions to languages mapping.
| } | ||
| this.logger = await Logger.child(this.getName()); | ||
| this.config = await Controller.getConfig(); | ||
| this.minimumTokens = EnvVariable.getEnvVariableAsNumber(this.ENGINE_ENUM, EnvVariable.CONFIG_NAME.MINIMUM_TOKENS) || ( await this.config.getMinimumTokens(this.ENGINE_ENUM) ); |
There was a problem hiding this comment.
Environment variable would override config value for MinimumTokens.
|
|
||
| // Let user know about file paths that could not be matched | ||
| if (unmatchedPaths.length > 0) { | ||
| uxEvents.emit('info-verbose', eventMessages.getMessage('info.unmatchedPathExtensionCpd', [unmatchedPaths.join(",")])); |
There was a problem hiding this comment.
Simplified logic in sortPaths to emit information in only one path.
| export enum CONFIG_NAME { | ||
| MINIMUM_TOKENS = 'Minimum_Tokens' | ||
| } |
There was a problem hiding this comment.
New enum to track config values that can be fetched through environment variables. I've also attempted to standardize the access in this new module.
| @@ -0,0 +1,22 @@ | |||
| public class SomeApex1 { | |||
There was a problem hiding this comment.
New code fixtures to test CpdEngine
| return process.env[envVariableName]; | ||
| } | ||
|
|
||
| export function getEnvVariableAsNumber(engine: ENGINE, configName: CONFIG_NAME): number { |
There was a problem hiding this comment.
I think this will currently return NaN when the environment variable isn't set or isn't a number.
Do we want it to return NaN if it's set, but not a number, but null if it isn't set?
Whatever we choose, can you add documentation describing the contract when the value isn't set or the value can't be parsed?
There was a problem hiding this comment.
Good catch. We don't have a way to control what value the user may set. I see a suggestion on online forums to clean up the string before parsing:
parseInt(text.replace(/\D/g, ""),10);
where \D is a non-digit. Will add documentation for this as well.
There was a problem hiding this comment.
@jbartolotta-sfdc Let me know what you think about the fix.
There was a problem hiding this comment.
@rmohan20 - this change looks good.
| * @returns true if the language was identifed and false if not | ||
| */ | ||
| private matchPathToLanguage(path: string, languageToPaths: Map<LANGUAGE, string[]>): boolean { | ||
| const ext = path.slice(path.lastIndexOf(".") + 1); |
There was a problem hiding this comment.
Should this lowercase ext in case the user has a file named foo.Java or something else unexpected?
There was a problem hiding this comment.
Missed this. Will fix it.
rmohan20
force-pushed
the
d/rm/cpdAddons
branch
from
c12bdd4
to
1ef2c56
Compare
|
In TS, aren't Detroit and public the same?
…On Thu, Aug 12, 2021, 5:36 PM rmohan20 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In test/lib/cpd/CpdEngine.test.ts
<#522 (comment)>
:
>
let langPathMap: Map<LANGUAGE, string[]> = new Map();
langPathMap.set(LANGUAGE.APEX, ['file.cls','file.trigger']);
langPathMap.set(LANGUAGE.VISUALFORCE, ['file.page','file.component']);
- expect((testEngine as any).sortPaths(targets)).to.eql(langPathMap);
+ expect(testEngine.sortPaths(targets)).to.eql(langPathMap);
Agree, a @testvisible annotation would've been perfect.
This method is only default visible and not public scoped. In fact, it
has less visibility than protected, which would be needed for
subclassing. It's visible here only because of the test module's location.
@jbartolotta-sfdc <https://github.com/jbartolotta-sfdc> Any thoughts to
help us weigh the options?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#522 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA653WDOYLZORFLROKRD34LT4REIHANCNFSM5B7VKMPA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
rmohan20
force-pushed
the
d/rm/cpdAddons
branch
from
a63b95c
to
986bb97
Compare
| @@ -52,22 +25,26 @@ describe('CpdEngine', () => { | |||
| const targets: RuleTarget[] = [{ | |||
| target: '', | |||
| paths: ['file.cls','file.trigger','file.page','file.py','file.component'] | |||
| }] | |||
| }]; | |||
|
|
|||
| let langPathMap: Map<LANGUAGE, string[]> = new Map(); | |||
| langPathMap.set(LANGUAGE.APEX, ['file.cls','file.trigger']); | |||
| langPathMap.set(LANGUAGE.VISUALFORCE, ['file.page','file.component']); | |||
| expect((testEngine as any).sortPaths(targets)).to.eql(langPathMap); | |||
There was a problem hiding this comment.
@jfeingold35 @jbartolotta-sfdc As per your suggestions, I've restored the as any call for sortPaths and made it a private method again. The remaining as any calls are on public methods and they are already accessible directly.
No description provided.