[Resistor Duo analyzer (#127)] Add feedback to extract the constant to the top level if defined locally #164
Conversation
|
Hello. Thanks for opening a PR on Exercism 🙂 We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in. You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch. If you're interested in learning more about this auto-responder, please read this blog post. Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it. |
|
@Cool-Katt wanna do a first run on a review? I am currently on annual leave so a bit slow! |
|
о7 i'll got on it boss, but I think it'll be tomorrow at the earliest. Real life got in the way a little. |
| @@ -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 | |||
There was a problem hiding this comment.
Jokes aside, we do need to add this to the website-copy repo, I don't know how that process works.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Here is the PR for the website-copy change.
src/analyzers/practice/resistor-color-duo/ResistorColorDuoSolution.ts
Outdated
Show resolved
Hide resolved
| solution to \`resistor-color\` can be used as helper method here. When using an | ||
| \`Array\` as colors source, in a years time, will the student recall why it's | ||
| the _index_ in that array? When using an \`Object\`, what does the value mean? | ||
| Re-using \`colorCode\` explains this in both cases. |
There was a problem hiding this comment.
Any particular reason for the removals?
There was a problem hiding this comment.
In this case the must_use_a_helper feedback would be replaced by the new prefer_extracted_top_level_constant feedback. This analyzer is built in such a way that it short-circuits on the first issue it finds, in this case, since this happens in the final stages of the process, so the variable _lastIssue gets the issue. At least, that's how I understand it.
There was a problem hiding this comment.
That being said, I'm not sure if everyone kept with the practive of updating the snapshots whenever they make a PR, so this batch of updates might relate to some behaviors that weren't introduced in this PR. But, I can't be sure. I'm still very unfamiliar with this repo. 😅
There was a problem hiding this comment.
But, I can't be sure. I'm still very unfamiliar with this repo
I think we always run the tests, so this must come from this PR.
At least, that's how I understand it.
It can actually have multiple comments, but we only output multiple "nice to change" or "alternative option" comments to not overwhelm the student[s].
There was a problem hiding this comment.
Thanks for clarifying. In that case I will give the updated snapshots another review, and please tell me if you see anything fishy. 😅
| @@ -1111,6 +1125,20 @@ class Entry { | |||
| logger.log(`~> constant is not optimal`) | |||
| return false | |||
| } | |||
|
|
|||
| private checkForShouldDefineTopLevelConstantIssue(): | |||
There was a problem hiding this comment.
I would have probably put this as a public method with the other ones at the top, for consistency.
Something like hasConstantDefinedInBody() maybe
There was a problem hiding this comment.
Definitely open to this, not married to the name. I try to always make my methods private if the intention is to not have them as part of the public API, but if consistency is important here then I don't mind changing it. Let's see what @SleeplessByte thinks. 😄
There was a problem hiding this comment.
I do like the proposed name. I changed that. 👍
Steffe-Dev
changed the title
The goal of this PR is to address a subset of issue #127, namely adding the feedback to extract the constant to the top level when it was defined locally to the Resistor Color Duo analyzer.
Context
The original issue posed two problems to solve:
This PR addresses (2), since it is an atomic piece of new functionality. After the resolution of this PR the state of the issue should be re-evaluated, since addressing (1) would, in my opinion, require a refactor, due to the entanglement of logic between the helper method feedback and the rest of the feedback, which should be weighed against the value gained.
Main changes
ShouldDefineTopLevelConstant) to denote the new kind of feedback.lastIssue_member of theEntryclass, such that it can be consumed in theindex.tsfile responsible for parsing the result of the Resistor Color Duo analyzer.PREFER_EXTRACTED_TOP_LEVEL_CONSTANTfactory defined in theGigasecondSolution.tsfile. This copy will have to be added to thewebsite_copyrepo.