Skip to content

Implementation of mmif rewinder#250

Closed
Kimmy-DohyunKim wants to merge 7 commits intodevelopfrom
247-rewinder
Closed

Implementation of mmif rewinder#250
Kimmy-DohyunKim wants to merge 7 commits intodevelopfrom
247-rewinder

Conversation

@Kimmy-DohyunKim
Copy link
Contributor

@Kimmy-DohyunKim Kimmy-DohyunKim commented Jan 18, 2024

@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

keighrim commented Jan 27, 2024

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

keighrim commented Jan 29, 2024

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
@keighrim keighrim restored the 247-rewinder branch May 27, 2025 15:08
@keighrim keighrim deleted the 247-rewinder branch June 5, 2025 19:27
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