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

make epub file extension replacement case-insensitive #2

Merged
merged 1 commit into from Oct 3, 2015

Conversation

mojavelinux
Copy link

No description provided.

@mojavelinux
Copy link
Author

Now if the user specifies -o foo.MOBI, the name of the final file with be foo.MOBI instead of foo.mobi.

@chloerei
Copy link
Owner

chloerei commented Oct 2, 2015

I think it make complex to understand what is mobi output result. Output name should base on target but not epub_file, epub_file is a temp file for mobi.

Problem come form epub converter support two format but only set outfilesuffix as one value .epub. I'm not complete read the converter call stack, but change it maybe too far for fix output filename.

I think current fix-out-file-option branch is good for fix -o problem.

@mojavelinux
Copy link
Author

I see your concern. Technically, the epub_file is derived from the target, so the two files have the same rootname, but I agree that it is clearer to rebuild the path off the target directly. I have updated my pull request. I think it satisfies your concern while still maintaining compatibility with existing behavior.

(I realize that we are still lacking tests...I am testing against editions to make sure that it continues to work. Clearly, we need to get tests setup...that's a major error in my part that I skipped that step).

@@ -459,7 +460,7 @@ def distill_epub_to_mobi epub_file, target
require 'kindlegen' unless defined? ::Kindlegen
kindlegen_cmd = ::Kindlegen.command
end
mobi_file = ::File.basename(target).sub EpubExtensionRx, '.mobi'
mobi_file = ::File.basename(target =~ MobiExtensionRx ? target : %(#{::Asciidoctor::Helpers.rootname target}.mobi))
Copy link
Author

Choose a reason for hiding this comment

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

Now we only substitute if the extension isn't .mobi. That's the one thing we are enforcing. The extension of the mobi file must be .mobi (case insensitive).

@chloerei
Copy link
Owner

chloerei commented Oct 2, 2015

I still think it's not necessary to have a guard to check target is end with /.mobi/i , .kf8 and .azw3 is also a valid KF8 extname. asciidoctor-pdf and kindlegen both don't prevent the use of any extname.

But I accept it that let things move on, it's not a big problem...

@mojavelinux
Copy link
Author

Now I see why you were replacing the epub extension with mobi. It took me a while to understand why you were doing it that way, but now I totally see it. Sorry it took me so long to make the connection.

@mojavelinux mojavelinux changed the title use original target if it contains mobi file extension make epub file extension replacement case-insensitive Oct 3, 2015
@mojavelinux
Copy link
Author

The only change that remains is to make the epub extension regex case-insensitive. Once that's merged in, then I think we'll finally have it sorted out.

chloerei added a commit that referenced this pull request Oct 3, 2015
make epub file extension replacement case-insensitive
@chloerei chloerei merged commit 5ba82a8 into chloerei:fix-out-file-option Oct 3, 2015
@chloerei
Copy link
Owner

chloerei commented Oct 3, 2015

Great!

@mojavelinux mojavelinux deleted the pr/31-review branch April 11, 2017 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants