-
Notifications
You must be signed in to change notification settings - Fork 8
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
Merge Develop to Master, release v5.29.0 #194
Conversation
… because it's needed for some unit tests
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #194 +/- ##
==========================================
+ Coverage 79.99% 80.09% +0.10%
==========================================
Files 64 65 +1
Lines 4958 5003 +45
==========================================
+ Hits 3966 4007 +41
- Misses 992 996 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
if !reasoning | ||
owlapi.disable_reasoner | ||
end | ||
owlapi = LinkedData::Parser::OWLAPICommand.new( |
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.
maybe there is a downgrade in the code here, why remove the usage of the helper owlapi_parser(logger: nil)
and replaced with LinkedData::Parser::OWLAPICommand.new
?
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.
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.
@syphax-bouazzouni, I just merged the code from your earlier pull request. From my understanding, the owlapi_parser(logger: nil)
method includes a call to unzip_submission(logger)
, which is already being run inside the process_submission
method on line 1005. That call had to be done earlier because your updated generate_rdf
method now requires the file_path
to be passed as an argument. So calling owlapi_parser(logger: nil)
inside generate_rdf
would duplicate the unzipping process. Again, I simply merged your earlier pull request and fixed any code that it had missed.
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.
@syphax-bouazzouni, I think we can simply remove the file_path
from the list of required arguments for generate_rdf
and then make a call to the owlapi_parser(logger: nil)
method inside generate_rdf
instead of the direct call to LinkedData::Parser::OWLAPICommand.new
. From what I can see this is the only place in the generate_rdf
that needs the file_path
.
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.
Yes removing that argument, would be the right solution here, even in our local branch generate_rdf
does not have the file_path
argument, I don't know why it ended up in my PR.
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'll make that change on our end and re-test
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, thanks, and sorry for the bother.
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.
the refactor that @mdorf suggested in #194 (comment) be implemented after this release is complete
Corrects the previous commit that left out the Gemfile
Release v5.29.0 includes: