-
Notifications
You must be signed in to change notification settings - Fork 120
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
[graal] Add support of Graal's CoLic Backend to ELK [File-level + Study] (WIP) #669
Conversation
Pull Request Test Coverage Report for Build 1581
💛 - Coveralls |
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.
Thank you @inishchith for the PR. Overall it looks OK. I see that some code of the study is similar to the one proposed here: #664. It would be possible to abstract the common logic to a class (e.g., study_evolution) and call it from cocom and colic enrichers?
A similar comment targets the file graal_utils
, it would be possible to wrap the variables in graal_utils
to methods, which use some variables to set up the query attributes? If this is possible, then we could include these methods in the study_evolution
file.
WDYT?
@valeriocos Yes. Agree with your thoughts. Will update the PR once I've worked on it. Thanks! |
60b2d0b
to
3a7df69
Compare
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.
Thank you @inishchith! Also this PR looks great. The comments are similar to the PR #664, plus there are some suggestions to reduce redundant code. Let me know what you think.
3e068d0
to
7fb9cc8
Compare
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 @inishchith to address my comments. Overall the PR looks great.
I'm not sure whether it's better to unify this PR with #664, since there are common pieces of code, and some modifications cannot be done. WDYT?
Thanks for the review @valeriocos.
Which would allow us to focus on Graal integrations as a whole and testing would become much easier. |
Add support of Graal's CoLic backend to ELK and it's corresponding tests Signed-off-by: inishchith <inishchith@gmail.com>
7fb9cc8
to
ac97963
Compare
You're welcome @inishchith and thanks for proposing the levels of commit :) I would add a 6. point to add the csvs of the colic and cocom (as reference you can have a look at the git.csv) |
closing in reference to #672 |
Address: inishchith/gsoc#9
TODO:
NOTE:
Graal
module not being a part ofrequirements.txt
(dependencies) and is a part of this PR just to check the coverage of new additions.Signed-off-by: inishchith inishchith@gmail.com