-
Notifications
You must be signed in to change notification settings - Fork 0
346 summarizer #350
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
346 summarizer #350
Conversation
… of the pull request
|
The checks keep failing. I think I got all type complaints from the tests fixed and removed unused code that was using non-existing methods. But I am unclear on how to deal with remaining issues. Many of them do seem like errors I have seen in the past where some attributes are not properly generated, but maybe they are all due to some type checking. For example, the first failure reported is This refers to the following piece of code def _init_nodes(self):
for doc in self.mmif.documents:
self.add_node(None, doc)
...Somehow the checker got the idea that self.mmif is a string. I need to understand the check better before I can fix this. |
|
All remaining warnings are from pytype and I do not know how to deal with some of them (see for example the one listed above). Do we have a policy on merging while some chacks fail? |
|
They are not warnings, but errors. I don't think we have a written policy on failing tests, but failing tests will make CI/CD pipelines fail and block any releases. Hence we can't merge a failing branch. |
| to trace nodes all the way up to the primary data.""" | ||
|
|
||
| def __init__(self, mmif): | ||
| self.mmif = mmif if type(mmif) is Mmif else Mmif(mmif) |
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 guess this should have a type hinting, so that pytype doesn't make the minimalist assumption (it being a str)
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.
Yeah, I figured I would have to do some more extended type hinting for those cases.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #350 +/- ##
============================================
- Coverage 85.81% 65.56% -20.25%
============================================
Files 15 22 +7
Lines 1903 2843 +940
============================================
+ Hits 1633 1864 +231
- Misses 270 979 +709
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Added and adapted the summarizer from https://github.com/clamsproject/mmif-summarizer/. Also added some notes on how to add a CLI script.