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.

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

Refactor cut and retain git blame history #820

Merged
merged 13 commits into from
Sep 25, 2022

Conversation

desh2608
Copy link
Collaborator

This should hopefully retain all (most?) of the git blame history. The trick is to create a new branch for each split file, and create the new file using git mv so that it retains the history of the original file. Once all the branches are created, we merge them together.

@pzelasko
Copy link
Collaborator

Thanks, amazing work! Please fix the failing style check and LGTM

@desh2608
Copy link
Collaborator Author

desh2608 commented Sep 25, 2022

Thanks, amazing work! Please fix the failing style check and LGTM

I think adding the imports required to resolve the failing checks would cause a circular dependency. See the dependency graph in __init__.py. I am not sure what's a good way to resolve this.

Update: Ideally, making the type hints as strings ("MonoCut" instead of MonoCut), i.e., what is called a forward reference should have been enough, but I don't know why flake8 is still not happy with it. One way to ignore the specific lines is to add a # noqa: F821 on the lines to ignore it.

@pzelasko
Copy link
Collaborator

IMO the easiest way to fix is to change the types from specific (like MixedCut) to generic (like Cut). It's still truthful even if it misses on some auto-hint capabilities if we know that there will be only a specific type returned. I think maybe this problem can be fixed on a higher level by adopting typing stub files (.pyi) but that's probably a considerably larger effort.

@desh2608
Copy link
Collaborator Author

Okay, I changed the MonoCut and MixedCut types to Cut. There were still 2 instances of CutSet so I add noqa to ignore them.

@pzelasko pzelasko added this to the v1.8 milestone Sep 25, 2022
@pzelasko pzelasko merged commit c879edd into lhotse-speech:master Sep 25, 2022
@desh2608 desh2608 deleted the cut_refactor branch September 25, 2022 14:48
@desh2608 desh2608 restored the cut_refactor branch September 25, 2022 16:28
@desh2608
Copy link
Collaborator Author

It seems the git history was not retained. If this is an issue, we can revert this merge and try to figure out how to do it.

@pzelasko
Copy link
Collaborator

Don't worry about it. I probably messed up because I squashed then merged.

@desh2608 desh2608 deleted the cut_refactor branch November 2, 2023 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants