Skip to content

Conversation

@Steffe-Dev
Copy link
Contributor

Add prefer_extracted_top_level_constant analyzer comment to Resistor Color Duo on the Javascript track.

The copy is heavily based on the existing copy for prefer_top_level_constant in the gigasecond file in the parent directory of the new file. I workshopped the copy I added a bit with ChatGPT with the hope of not overexplaining, but I am very open to all suggestions.

Relates to (exercism/javascript-analyzer#127)
This is a companion to this PR.

SleeplessByte
SleeplessByte previously approved these changes Mar 5, 2025
Copy link
Member

@SleeplessByte SleeplessByte left a comment

Choose a reason for hiding this comment

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

The suggestion reformats this to the markdown style guide we use for Exercism. Everything else looks pristine.

@SleeplessByte SleeplessByte requested a review from a team March 5, 2025 06:57
…ed_top_level_constant.md

Co-authored-by: Derk-Jan Karrenbeld <[email protected]>
@Steffe-Dev
Copy link
Contributor Author

The suggestion reformats this to the markdown style guide we use for Exercism. Everything else looks pristine.

Thanks, will take it into account in the future. 🙏 I commited your suggested changes.

const %{name} = %{value}

// the rest of your code below it
export const decodedValue = (...)
Copy link
Member

Choose a reason for hiding this comment

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

I just realised, this may NOT be the function declaration the student is using. It could be many forms, including:

export function decodedValue(...)

Ideally, we inject the function signature the student actually used here. Do you think that's something you could add? We do it in another place, where it's using the source code to extract this part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion. I added it, both here and on the analyzer pr. 🙌

@Steffe-Dev
Copy link
Contributor Author

@SleeplessByte Just a reminder of this PR, since the analyzer one has been merged. 😄

@SleeplessByte SleeplessByte merged commit 1ab80fd into exercism:main Mar 13, 2025
1 check passed
@SleeplessByte
Copy link
Member

Woop woop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

track/javascript JavaScript track type/analyzer-comments Analyzer comments

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants