-
Notifications
You must be signed in to change notification settings - Fork 91
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
Fix references to morituri. #109
Fix references to morituri. #109
Conversation
There are a number of lines with
It feels wrong to just replace Edit: I replaced those lines with just "Whipper". See 2524b11 |
@@ -1,10 +1,10 @@ | |||
GOOD=/home/thomas/gst/git/gst-plugins-good/gst/audioparsers | |||
MORI=/home/thomas/dev/own/morituri | |||
WHIP=/home/thomas/dev/own/whipper |
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.
Fixed reference to thomas’ PATH don’t look right anyway. What is this file? Just a bisect script he use(d) for some bugs?
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, seems so.
I think that in the near future we'll probably rm -r etc/bash_completion.d
(doc/
replaced (?) and there's also lots of cruft in misc/
).
@JoeLametta shall we merge this one soon and prep a minor release? Might be useful for packagers. EDIT: When it is ready to be merged and not just WIP, of course. |
I will try to catch up on this this week. |
👍 |
Rebased on current master now that #130 has gone in. There are a number of morituri references left (
|
Correct.
;)
It could be changed in the future but, right now, that's still true (so 'morituri' can be replaced with 'whipper' in that comment).
OK
👍 |
Changed the text in |
whipper/test/common.py
Outdated
'REM COMMENT "Morituri.*', | ||
'REM COMMENT "Morituri %s"' % (morituri.__version__), | ||
'REM COMMENT "Whipper.*', | ||
'REM COMMENT "Whipper %s"' % (whipper.__version__), | ||
ret, re.MULTILINE) |
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 think the original code is broken here. All the cue sheets used in the tests have 'morituri' in all lower case (and so does the code that generates those lines). Since the regular expression doesn't ignore case, that means the substitution currently has no effect.
It doesn't have to be fixed in this PR, but it should be fixed (by making it "whipper" rather than "Whipper").
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.
Good catch.
Yeah, it looks like a mistake: I'll fix it in a separate commit.
Thanks
Note that the (internal) file names of "whipper/test/cache/result/fe105a11.pickle" did not get updated, since that breaks the tests for some reason.
> Loaded and re-saved in a different pickle format (the initial format). > After that it's just search/replace
Looks good to me! Thanks. |
Just adding so I can see my progress and get automated testing feedback. Started off as trying to fix #100, but then I started getting carried away. :)
See #109 (comment) for details.