-
Notifications
You must be signed in to change notification settings - Fork 80
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
Added Eda to Market Trend Classification Model #206
Conversation
Thank you for submitting your pull request! 🙌 We'll review it as soon as possible. In the meantime, please ensure that your changes align with our chaotic CONTRIBUTING.md. If there are any specific instructions or feedback regarding your PR, we'll provide them here. Thanks again for your contribution! 😊 |
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.
@SimranShaikh20 Please link the issue using keywords. (fixes/closes..) You can find the detailed info about the same here:
https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword
If needed, please check out merged PRs for reference.
Thanks & regards
@Mayureshd-18 i have added fixes and close tag |
@rohitinu6 pls check PR and merged it ! |
@rohitinu6 can you pls merged this pr! |
@jvedsaqib pls check PR and merged it . |
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.
@SimranShaikh20 Please use the correct keywords to link the issue. It should be visible on PRs page. Please refer to other merged PRs for reference.
Thanks and regards
@jvedsaqib Checked the code. Lgtm. See on your side pls. |
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.
commited
@Mayureshd-18 |
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.
@SimranShaikh20 As you can see that even after your changes this PR is not linked to the issue correctly as the issue cant be seen on the Prs page directy,
How about you try to add just Closes <issueno.>
in the description directly?
Thanks
Apologize for inconvenience I had make changes pls check and merged it |
@SimranShaikh20 I edited your description. Now as you can see the issue is linked with the PR correctly. Adding the Thanks |
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.
Looks good to me!
Will merge immediately after atleast one more review! Thanks & regards |
Okay |
Pls add other label like gssoc ext |
@rohitinu6 pls review to PR and is there any conflicts let me know |
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.
@rohitinu6 apolozies for inconvience update branch and synced repo |
@SimranShaikh20 it's not done yet |
@rohitinu6 i will make new PR may there is some technical issue from my side |
fixes - #202
Issue no - #202
Closes #202
I have added eda to market trend classification !
@rohitinu6 pls review it and if there is any conflicts let me know !