-
-
Notifications
You must be signed in to change notification settings - Fork 20
[Resistor Duo analyzer (#127)] Add feedback to extract the constant to the top level if defined locally #164
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
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. |
src/analyzers/practice/resistor-color-duo/ResistorColorDuoSolution.ts
Outdated
Show resolved
Hide resolved
src/analyzers/practice/resistor-color-duo/ResistorColorDuoSolution.ts
Outdated
Show resolved
Hide resolved
Cool-Katt
left a comment
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.
Preliminary report: looks good to me.
src/analyzers/practice/resistor-color-duo/ResistorColorDuoSolution.ts
Outdated
Show resolved
Hide resolved
SleeplessByte
left a comment
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 am still on holiday.
Basis of this PR is perfect, notes are about the style of the code flow and suggestions how to fix that.
src/analyzers/practice/resistor-color-duo/ResistorColorDuoSolution.ts
Outdated
Show resolved
Hide resolved
src/analyzers/practice/resistor-color-duo/ResistorColorDuoSolution.ts
Outdated
Show resolved
Hide resolved
SleeplessByte
left a comment
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.
Yeah flows better I think.
I don't know what will happen in a case where this tip (suggestion) is added and something else is wrong, but not that concerned about it. Will it still suggest to use a helper function? Because it shouldn't.
Regarding my comment in website copy, I think this is the line that injects the function signature:
| 'method.signature': solution.entry.signature, |
...so you can reuse that. I think we have export ${'signature'} or something like that in the tip that tells you to inline the export
|
Regarding what will happen when other things are wrong, I updated the snapshot tests with my first iteration, and then after this flow change all the tests still passed, which includes not suggesting a helper function when this new tip is raised. It makes sense, because my code does an early exit before we reach the part of the analysis that checks for the presence of a helper function, and that didn't change with this iteration. But please feel free to check out the branch and run them yourself if you are curious. 😄 Good catch on showing their exact export. I will make it happen. 🙌 |
SleeplessByte
left a comment
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.
Looking great.
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.