-
Notifications
You must be signed in to change notification settings - Fork 511
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
Fix part of #210 : Add Tests for NumericInputIsLessThanOrEquaToRuleClassifierProvider #1775
Fix part of #210 : Add Tests for NumericInputIsLessThanOrEquaToRuleClassifierProvider #1775
Conversation
update fork approved
update local copy
update fork approved
update develop
…nto develop update
update my develop
… develop update fork
update fork
@rt4914 PTAL |
Adding @anandwana001 and @aggarwalpulkit596 as I won't be able to review this today. |
...org/oppia/domain/classify/rules/NumericInputIsLessThanOrEqualToRuleClassifierProviderTest.kt
Show resolved
Hide resolved
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.
LGTM, Thanks @prayutsu
@prayutsu i would suggest to clean up your fork/ read a bit about GitHub. In every of your PR i see a lot of merge commits it won't affect anything but doing it right way would make it a bit cleaner. |
Okay, so should I re-fork the repo? |
@anandwana001 @aggarwalpulkit596 Should I add tests for remaining numericInput rules as well? |
If you have issues with your own develop in that case yes you should re
fork if there aren't issues with develop in that case i guess you need to
things the right way.
…On Thu, Sep 3, 2020, 1:03 PM Abhay Garg ***@***.***> wrote:
@prayutsu <https://github.com/prayutsu> i would suggest to clean up your
fork/ read a bit about GitHub. In every of your PR i see a lot of merge
commits it won't affect anything but doing it right way would make it a bit
cleaner.
Okay, so should I re-fork the repo?
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#1775 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AG6KGSVPKZT642TPWHDVIX3SD5BDNANCNFSM4QTSM5XA>
.
|
I suggest completing the pending first - #1740 |
Actually, I am waiting for the review of @rajat Talesra
<rajattalesra4914@gmail.com>, he told me that he's busy this week, so he'll
review that PR next week. So as soon as he reviews it, I'll start working
on it again.
…On Thu, Sep 3, 2020 at 1:08 PM Akshay Nandwana ***@***.***> wrote:
@anandwana001 <https://github.com/anandwana001> @aggarwalpulkit596
<https://github.com/aggarwalpulkit596> Should I add tests for remaining
numericInput rules as well?
I suggest completing the pending first - #1740
<#1740>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1775 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANA273MBHT64HA6LLAHARC3SD5BXDANCNFSM4QTSM5XA>
.
|
ah, Okay then. You can go ahead with other numeric issues. |
So I should squash my multiple commits into one. Right? |
Thank you!
On Thu, 3 Sep 2020, 1:14 pm Akshay Nandwana, <notifications@github.com>
wrote:
… Actually, I am waiting for the review of @rajat <https://github.com/rajat>
Talesra ***@***.***, he told me that he's busy this week,
so he'll review that PR next week. So as soon as he reviews it, I'll start
working on it again.
… <#m_-4096679496257126558_>
On Thu, Sep 3, 2020 at 1:08 PM Akshay Nandwana *@*.***> wrote:
@anandwana001 <https://github.com/anandwana001>
https://github.com/anandwana001 @aggarwalpulkit596
<https://github.com/aggarwalpulkit596>
https://github.com/aggarwalpulkit596 Should I add tests for remaining
numericInput rules as well? I suggest completing the pending first - #1740
<#1740> <#1740
<#1740>> — You are receiving
this because you were mentioned. Reply to this email directly, view it on
GitHub <#1775 (comment)
<#1775 (comment)>>,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ANA273MBHT64HA6LLAHARC3SD5BXDANCNFSM4QTSM5XA
.
ah, Okay then. You can go ahead with other numeric issues.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1775 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANA273KCC2PFJBLGBUFWQILSD5CM3ANCNFSM4QTSM5XA>
.
|
No @prayutsu it's.not what i meant we are already squashing your commits but the point is that if you see.your git history it's saying that there are 22 commits but you made only 2 commits so it should be only 2 comments in most.of the cases. Does that make sense ? |
Actually they are those commits by which I update my develop branch with
the original repo. So, I think there's some better way that could do the
same?
Can you tell me how can I clean it up or link some references?
I found this - clean-up-your-fork
<https://stackoverflow.com/questions/6102297/how-do-i-clean-up-my-github-fork-so-i-can-make-clean-pull-requests>
…On Thu, 3 Sep 2020, 1:18 pm Pulkit Aggarwal, ***@***.***> wrote:
No @prayutsu <https://github.com/prayutsu> it's.not what i meant we are
already squashing your commits but the point is that if you see.your git
history it's saying that there are 22 commits but you made only 2 comments.
So it should be only 2 comments in most.of the cases. Does that make sense ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1775 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANA273K62ZETZUMOJHRHVLTSD5C4XANCNFSM4QTSM5XA>
.
|
…uleClassifierProvider (oppia#1775) * check fork * local develop branch synced with remote branch * Added tests for NumericInputIsLessThanOrEqualToRuleClassifierProvider * Added more tests for Integers
Explanation
Fixes part of #210
Added tests for
NumericInputIsLessThanOrEquaToRuleClassifierProvider.kt
Checklist