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

Initial support for Windows #3

Merged
merged 2 commits into from
Feb 7, 2017
Merged

Initial support for Windows #3

merged 2 commits into from
Feb 7, 2017

Conversation

klvbdmh
Copy link
Contributor

@klvbdmh klvbdmh commented Feb 7, 2017

References: #2

Only _str_to_phonems functionality for now.

@hadware
Copy link
Owner

hadware commented Feb 7, 2017

Alright, fine with me :)

Will you be updating the other class methods (_str_to_audio and phomems_to_audio) so they match the the new way of setting up arguments you introduced? (much appreciated btw)

@hadware hadware merged commit b5fd7d0 into hadware:master Feb 7, 2017
@klvbdmh
Copy link
Contributor Author

klvbdmh commented Feb 8, 2017

Yes I will. I only did one class method just to see if the format is fine and if there are no regressions on Linux. Now that I know how you fixed my code, I'll apply those fixes in other methods.

One question: why is it necessary to wrap phonem_synth_args in ' '.join? subprocess.run() accepts a list as its first argument and I use it like so in my scripts on various Linux servers I manage. Is there some sort of regression with a list that doesn't happen with a single string?

@hadware
Copy link
Owner

hadware commented Feb 8, 2017

Yes I will. I only did one class method just to see if the format is fine and if there are no regressions on Linux. Now that I know how you fixed my code, I'll apply those fixes in other methods.

Good. Very good.

One question: why is it necessary to wrap phonem_synth_args in ' '.join?

I genuinely have no idea. I thought so as well, and I checked and double checked, but when I pass the args as a list, the run command doesn't return anything. I'm suspecting it might be because of this:

If args is a sequence, the first item specifies the command string, and any additional items will be treated as additional arguments to the shell itself.

(extracted from http://stackoverflow.com/a/15109975/1327143 which itself was extracted from the doc)

and here, the first argument is a MALLOC_CHECK=0 which might screw things up a bit.

@klvbdmh
Copy link
Contributor Author

klvbdmh commented Feb 8, 2017

BTW I figured out how to get mbrola.exe binary - it's inside a DOS archive that has to be downloaded separately. I updated the installation instructions in my fork. I also successfully piped espeak and mbrola together to create a playable .wav file. I have to iron out a few details and I'll push the updates tomorrow.

I thought so as well, and I checked and double checked, but when I pass the args as a list, the run command doesn't return anything

What happens when you omit shell=True?

@hadware
Copy link
Owner

hadware commented Feb 8, 2017

I get

FileNotFoundError: [Errno 2] No such file or directory: 'MALLOC_CHECK_=0'

I mean, I know passing a string isn't standard practice, but for this particular case I think we can ignore it. It's good to know that you've cracked the mbrola.exe thing. I'll try creating more unittests in the meantime to prevent any regressions on linux.

@klvbdmh
Copy link
Contributor Author

klvbdmh commented Feb 8, 2017

All right. String it is.

Any preferences about max line length? I've noticed you're using 120, which is fine by me.

When you're writing more tests, can you add TestPhonemsToAudio? Right now we have Str -> Audio and Str -> Phonems.

@hadware
Copy link
Owner

hadware commented Feb 9, 2017

Max line length? You mean, lines in the code? If that's what you're talking about, unless i've made a mistake, i'm trying as hard as I can to conform to PEP8. Else, must be a mistake.

I'll indeed write this missing unittest, thanks for pointing that out.

@klvbdmh
Copy link
Contributor Author

klvbdmh commented Feb 9, 2017

Yes, line length in the code. PEP8 recommends 80, but 120 is a sensible option too; it's what I use in my projects.

@hadware
Copy link
Owner

hadware commented Feb 10, 2017

Oh, I'm confirming to PEP8 in all cases, since Pycharm's automatic PEP8 checker immediatly screams at me when I don't respect it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants