Skip to content
This repository has been archived by the owner on Jan 12, 2023. It is now read-only.

Changing default ordering of extensions for fetching ebooks #103

Merged
merged 1 commit into from
May 18, 2018

Conversation

lissyx
Copy link
Contributor

@lissyx lissyx commented May 16, 2018

Resolves #102

@lissyx lissyx force-pushed the change-extensions-order branch 6 times, most recently from 8e95792 to b7c5136 Compare May 16, 2018 11:10
Copy link
Owner

@c-w c-w left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request! Could you please also add a docstring explaining the variable and when it should be set?

@lissyx lissyx force-pushed the change-extensions-order branch from b7c5136 to 35ec513 Compare May 16, 2018 11:53
@lissyx
Copy link
Contributor Author

lissyx commented May 16, 2018

@c-w Done, tell me if the wording is okay to you :)

@lissyx
Copy link
Contributor Author

lissyx commented May 16, 2018

@c-w I'm also adding the .encoding change :)

@lissyx lissyx force-pushed the change-extensions-order branch from 35ec513 to 5da8e67 Compare May 16, 2018 12:43
@MasterOdin
Copy link
Collaborator

I'd ask that you add a --ascii or --prefer-ascii (with shorthand -a) into the argparse for this function.

@lissyx
Copy link
Contributor Author

lissyx commented May 16, 2018

Sure, I'll add that.

@lissyx lissyx force-pushed the change-extensions-order branch from 5da8e67 to e7b5e65 Compare May 16, 2018 14:59
@lissyx
Copy link
Contributor Author

lissyx commented May 16, 2018

@MasterOdin Re-pushed with the command line argument :-)

@lissyx lissyx force-pushed the change-extensions-order branch from e7b5e65 to 2cac5ed Compare May 16, 2018 15:26
MasterOdin
MasterOdin approved these changes May 16, 2018

extensions = text._format_download_uri(12345, prefer_ascii=True)
self.assertEqual(extensions.split('/')[-1], ascii_filename)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a test where the files are ['12345-8.txt', '12345.txt'] so that we have a test to verify that we prefer -8 over nothing by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, should be addressed now :)

@MasterOdin
Copy link
Collaborator

Looks good!

We'll want to bump the version to 0.7.0 for this since it's a slight backwards breaking change.

@lissyx lissyx force-pushed the change-extensions-order branch from 2cac5ed to c553017 Compare May 16, 2018 18:23
@lissyx
Copy link
Contributor Author

lissyx commented May 16, 2018

@MasterOdin If you think we should not break backward compatibility, I'd be happy to change the behavior so that the default path stays the same as before :)

@MasterOdin
Copy link
Collaborator

I'm fine breaking it and this is probably preferable for most use cases, it's just something to be aware of and that it should be noted on the release page when a new release is cut.

@lissyx
Copy link
Contributor Author

lissyx commented May 18, 2018

Then I guess someone needs to merge it, if it's good for you :-)

@lissyx lissyx force-pushed the change-extensions-order branch from c553017 to e7c73a2 Compare May 18, 2018 08:56
@lissyx
Copy link
Contributor Author

lissyx commented May 18, 2018

Re-pushed, updating the version field.

@c-w c-w merged commit 36cc023 into c-w:master May 18, 2018
@c-w
Copy link
Owner

c-w commented May 18, 2018

Thanks a lot for the PR, @lissyx!

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

Successfully merging this pull request may close these issues.

3 participants