Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implementation of mmif rewinder #250

Closed
wants to merge 7 commits into from
Closed

Implementation of mmif rewinder #250

wants to merge 7 commits into from

Conversation

Kimmy-DohyunKim
Copy link

@codecov Codecov
Copy link

codecov bot commented Jan 18, 2024 •
edited
Loading

Codecov Report

Attention: 58 lines in your changes are missing coverage. Please review.

Comparison is base (226ddf4) 91.82% compared to head (6de4fa3) 87.07%.

Files Patch % Lines
mmif/utils/rewinder.py 10.76% 58 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop     clamsproject/mmif-python#250      +/-   ##
===========================================
- Coverage    91.82%   87.07%   -4.76%     
===========================================
  Files            8        9       +1     
  Lines         1040     1106      +66     
===========================================
+ Hits           955      963       +8     
- Misses          85      143      +58
Flag Coverage Δ
unittests 87.07% <12.12%> (-4.76%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

keighrim
keighrim requested changes Jan 22, 2024
View reviewed changes
Copy link
Member

@keighrim keighrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, and should be functional. But I think the PR can be improved with more polishing and adding unit tests.

@Kimmy-DohyunKim
Options are added: -o for output file name and -p for pretty printing.
a2b75be
@Kimmy-DohyunKim
Review comments reflected:
f173a37 
- changed rewinder file name taking off mmif_.
- changed _delete_last function name to be single underscored.
- added CLI argument for number of views to rewind.
- changed read_mmif from json.load() -> file.read()
@keighrim
Copy link
Member

After giving some thought on the right place for the invocation (entrypoint) of the rewinder code, now I think the rewinder.py part of this PR can be moved to clams-python package since we already have a CLI helper in clams-python that makes adding "rewinder" as an additional CLI subcommand for clams much easier (than re-doing that in this mmif-pyhon package). I'll be splitting this PR and start a new PR in the clams-python repo.

@keighrim
updated rewinder module ...
6de4fa3 
* optimized MMIF file I/O
* added rewinding by number of apps (opposed to number of views, not exposed in CLI)
@keighrim
Copy link
Member

closing without merge as the issue is moved to clams-python repo (clamsproject/clams-python#190).

@keighrim keighrim closed this Jan 29, 2024
@keighrim keighrim mentioned this pull request Jan 29, 2024
Merged
@keighrim keighrim removed their request for review January 29, 2024 02:49
@keighrim keighrim deleted the 247-rewinder branch January 30, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keighrim keighrim keighrim requested changes

Assignees
No one assigned
Labels
None yet
Projects
infra
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MMIF rewinder
2 participants
@Kimmy-DohyunKim @keighrim