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

[Resistor Duo analyzer (#127)] Add feedback to extract the constant to the top level if defined locally #164

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
ExtractedVariable,
extractExports,
extractFunctions,
extractVariables,
findAll,
findFirst,
findTopLevelConstants,
Expand Down Expand Up @@ -73,13 +74,18 @@ export class UnexpectedCallFound {
) {}
}

export class ShouldDefineTopLevelConstant {
constructor(public readonly name: string, public readonly value: string) {}
}

type Issue =
| undefined
| MissingExpectedCall
| HelperNotOptimal
| MethodNotFound
| HelperCallNotFound
| UnexpectedCallFound
| ShouldDefineTopLevelConstant

class Constant {
public readonly name: string
Expand Down Expand Up @@ -508,6 +514,15 @@ class Entry {
}
}

if (!constant) {
const issue = this.hasConstantDefinedInBody()
if (issue instanceof ShouldDefineTopLevelConstant) {
logger.log('~> found a constant that was not declared at the top level')
this.lastIssue_ = issue
return false
}
}

if (this.hasOneMap) {
logger.log('~> is a map solution')
return this.isOptimalMapSolution(logger, this.body, constant, program)
Expand Down Expand Up @@ -1111,6 +1126,18 @@ class Entry {
logger.log(`~> constant is not optimal`)
return false
}

private hasConstantDefinedInBody(): ShouldDefineTopLevelConstant | undefined {
const localConstants = extractVariables(this.body).filter(
(constant) =>
constant.init?.type === AST_NODE_TYPES.ArrayExpression ||
constant.init?.type === AST_NODE_TYPES.ObjectExpression
)
if (localConstants.length) {
const nameOfFirstConstant = localConstants[0].name || 'COLORS'
return new ShouldDefineTopLevelConstant(nameOfFirstConstant, '...')
}
}
}

export class ResistorColorDuoSolution {
Expand Down
23 changes: 23 additions & 0 deletions src/analyzers/practice/resistor-color-duo/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
MethodNotFound,
MissingExpectedCall,
ResistorColorDuoSolution,
ShouldDefineTopLevelConstant,
UnexpectedCallFound,
} from './ResistorColorDuoSolution'

Expand Down Expand Up @@ -120,6 +121,21 @@ const ISSUE_UNEXPECTED_CALL = factory<'unexpected' | 'expected'>`
CommentType.Actionable
)

const PREFER_EXTRACTED_TOP_LEVEL_CONSTANT = factory<'value' | 'name'>`
Instead of defining the constant _inside_ the function, consider extracting it

Choose a reason for hiding this comment

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

Missing emoji?
lol, jk.

Copy link
Author

@Steffe-Dev Steffe-Dev Feb 19, 2025

Choose a reason for hiding this comment

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

Jokes aside, we do need to add this to the website-copy repo, I don't know how that process works.

Copy link
Member

Choose a reason for hiding this comment

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

Open a PR on website-copy with the copy 😁

Btw, the emojis are there to quickly be able to detect what kind of comment it is when testing the analyzer.

Copy link
Author

Choose a reason for hiding this comment

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

Cool, I will do so. 👍

I copied this one directly from the GigasecondSolution.ts file, with minor modifications. Would you prefer if I added an emoji?

Copy link
Author

Choose a reason for hiding this comment

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

Here is the PR for the website-copy change.

to the top-level. Constants, functions, and classes that are not \`export\`ed,
are not accessible from outside the file.

\`\`\`javascript
const ${'name'} = ${'value'}

export const decodedValue = (...)
\`\`\`
`(
'javascript.resistor-color-duo.prefer_extracted_top_level_constant',
CommentType.Actionable
)

type Program = TSESTree.Program

export class ResistorColorDuoAnalyzer extends IsolatedAnalyzerImpl {
Expand Down Expand Up @@ -275,6 +291,13 @@ export class ResistorColorDuoAnalyzer extends IsolatedAnalyzerImpl {
}

output.disapprove()
} else if (lastIssue instanceof ShouldDefineTopLevelConstant) {
output.add(
PREFER_EXTRACTED_TOP_LEVEL_CONSTANT({
name: lastIssue.name,
value: lastIssue.value,
})
)
} else {
this.logger.error(
'The analyzer did not handle the issue: ' + JSON.stringify(lastIssue)
Expand Down
Loading