Skip to content

Allow inverted charset - #13

Merged
suminb merged 2 commits into
suminb:developfrom
joelnb:allow-inverted-charset
Jul 31, 2018
Merged

suminb merged 2 commits into
suminb:developfrom
joelnb:allow-inverted-charset

Conversation

@joelnb

@joelnb joelnb commented Jul 29, 2018

Copy link
Copy Markdown
Contributor

Allow easily using an inverted charset 0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ - some other implementations online (e.g. this Java one) use the charset in a different order. This code provides a new constant for this type of charset & allows the user to specify which charset to use on all calls (while leaving the current defaults unchanged).

I originally wrote this based on the develop branch but that has quite a few commits which are not in master so I changed it to be against master. Happy to rebase if you would prefer this PR go to develop instead.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+1.9%) to 91.228% when pulling 82a0e2d on joelnb:allow-inverted-charset into 1d1a9d7 on suminb:master.

@coveralls

coveralls commented Jul 29, 2018

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.03%) to 98.246% when pulling 44331f7 on joelnb:allow-inverted-charset into 1bb3a24 on suminb:develop.

@suminb

suminb commented Jul 30, 2018

Copy link
Copy Markdown
Owner

Looks good to me. Thanks for your work! I'd like to make a couple of requests though:

  • Submit this PR to develop branch
  • Fix ValueError: embedded null byte exceptions in Python 3.6

@joelnb
joelnb force-pushed the allow-inverted-charset branch from 82a0e2d to aabfaae Compare July 30, 2018 07:00
@joelnb
joelnb changed the base branch from master to develop July 30, 2018 07:05
@joelnb

joelnb commented Jul 30, 2018

Copy link
Copy Markdown
Contributor Author

Hi @suminb thanks for your original work on this also - saved me a lot of time!

I've changed the PR to be against develop but would you mind explaining what you mean by the second part? I've not come across that exception so far using this library & I've run the tests both before & after the rebase to develop.

@suminb

suminb commented Jul 30, 2018

Copy link
Copy Markdown
Owner

would you mind explaining what you mean by the second part?

Your build was failing when you first submitted this PR, but it appears that the issue has been resolved somehow 🤔

Now GitHub is telling me there are conflicts in base62.py. If you could resolve this, we're good to go.

@joelnb
joelnb force-pushed the allow-inverted-charset branch from aabfaae to 7fb58d0 Compare July 30, 2018 16:31
@joelnb
joelnb force-pushed the allow-inverted-charset branch from 7fb58d0 to 44331f7 Compare July 30, 2018 16:34
@joelnb

joelnb commented Jul 30, 2018

Copy link
Copy Markdown
Contributor Author

I've resolved the conflict now but thought it's worth mentioning what the conflict was - I had changed the __version__ from 0.3.2 to 0.4.0 (to denote the relatively large change) but you have now released 0.3.3. I resolved the conflict to keep the 0.4.0 version I originally added but happy to back that out & leave as 0.3.3 if you prefer.

@joelnb

joelnb commented Jul 30, 2018

Copy link
Copy Markdown
Contributor Author

Also I had a look at the travis issue you mentioned - I can't reproduce that & it has not happened this time so not sure about the reasons for that one.

@suminb

suminb commented Jul 31, 2018

Copy link
Copy Markdown
Owner

I've resolved the conflict now but thought it's worth mentioning what the conflict was - I had changed the version from 0.3.2 to 0.4.0 (to denote the relatively large change) but you have now released 0.3.3.

Correct. I should've informed you about that. My apology for the inconvenience.

I resolved the conflict to keep the 0.4.0 version I originally added but happy to back that out & leave as 0.3.3 if you prefer.

We (in this project, we mostly refers myself) generally change the version info in a release branch. No need to revert your code back though. I'll try to write up some guideline with regard to submitting PRs and enforce the rules thereafter.

@suminb
suminb merged commit ab786ae into suminb:develop Jul 31, 2018
suminb added a commit that referenced this pull request May 1, 2022
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.

3 participants