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
Rule no watch #51
Rule no watch #51
Conversation
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.
Thanks for your contribution, it's a great help 💙
I've left some questions about the code and the documentation.
In general, I like it. I think we can release this rule in 0.4.0 on the next week.
@@ -0,0 +1,7 @@ | |||
const event = { |
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.
Let's add some smoke cases to correct test. E.g. sample({ ... })
, `forward({...}), etc.
Made all necessary changes. |
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.
Great job!
I think, we are ready to merge it. I've left some comments about test-cases. Once you fix it, I'll merge PR.
.map(readExampleForTheRule) | ||
.map((result) => ({ | ||
...result, | ||
errors: [ |
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.
This pattern leads to hard debugging. Could you split incorrect test-cases and put it by files with only one error?
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.
Do you mean that for every incorrect case there should be a separate file?
1 error per file
For example: effects has 5 cases => 5 files
Am I right?
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.
Hm, in this case up to you. I think, it's easier to debug in case of broken tests, but I can be wrong here.
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.
You right. I choose a solution in the middle. No more than 2 errors per case.
Nice. I'll try to fix it asap 🙂 |
Let me know, when you'll be ready to merge. For me, this PR looks good. |
I'm ready to merge🙃 |
Thanks! Will be released in a week with |
Thank u too! |
close #48