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

base64 decoding displays sentinel instead of incorrect character #11

Merged
merged 3 commits into from
Nov 16, 2014
Merged

base64 decoding displays sentinel instead of incorrect character #11

merged 3 commits into from
Nov 16, 2014

Conversation

mzaffalon
Copy link
Contributor

It is mainly commit 70c83b3. It fixes one bug and a minor problem:

  • during base64 decoding, in the main loop, the error message displays the sentinel instead of the offending char
  • makes base64lookup table Uint8 instead of Uint

The second commit is because the code retouching went a bit out of control.

Disclaimer: I have never done a PR, so my apologies if I am doing this wrong. And I am new to Julia too...

also makes base64lookup Uint8 and removes the need to add 1 for the
decoding
@mzaffalon
Copy link
Contributor Author

By the way, I think 70c83b3 fixes also #10.

@mzaffalon
Copy link
Contributor Author

Doh, I missed the obvious change.

@timholy
Copy link
Collaborator

timholy commented Nov 15, 2014

I know @dcjones needs to take a break from using his hands, but perhaps I can help here? I'm not a Base64 coding guru, so I might need a little extra explanation. Can you give an example of the behavior that was broken before this PR, and how it now works?

If you do your development in a branch, you can easily switch back and forth. Maybe like this:

git checkout master  # make sure you are on master and have these commits
git checkout -b mz/sentinel   # Create a new branch holding these same commits. mz for your initials, sentinel for the topic.
git checkout master # to go back to master
git reset --hard origin/master  # make your master equal to the one on GitHub (deletes these commits from your master, but they're still on mz/sentinel)

Run your tests that should fail and post the results here. Then do git checkout mz/sentinel and rerun the same tests.

If at some point you want to make more commits to this branch, you can add them to mz/sentinel. Since you've already started a PR from your account's master, you can push to a different remote branch.

@timholy
Copy link
Collaborator

timholy commented Nov 15, 2014

Even better would be to add your example to the tests.

Also, it looks like I will have authority to merge this. Your changes overall look fine, and might even improve performance as well.

@mzaffalon
Copy link
Contributor Author

Thank you for the clear instructions but I think I still messed up.

Here is how the orginal code fails and how the PR fixes it:

using Codecs
valid_enc = encode(Base64, "hello world")
invalid_enc = copy(valid_enc)
invalid_enc[3] = ']'
decode(Base64, invalid_enc)

']'is not a valid Base64 character. The output of the original code is

ERROR: Invalid base64 symbol: �
 in error at error.jl:21

with the proposed change, the output is instead

ERROR: Invalid base64 symbol: ]
 in error at error.jl:21

I am not sure how to add this into a test since both the original code and the PR throw an error. It is the error message that changes.

@timholy
Copy link
Collaborator

timholy commented Nov 16, 2014

Thanks for the explanation, that's very helpful.

timholy added a commit that referenced this pull request Nov 16, 2014
base64 decoding displays sentinel instead of incorrect character
@timholy timholy merged commit 975e47e into BioJulia:master Nov 16, 2014
@mzaffalon
Copy link
Contributor Author

Thank you for taking care of it so quickly.

And because you are the one always preaching about @timeing the code, I tried, but no, I could not see any improved performance, or if there was one, it was hidden in the execution time statistical fluctuations for encoding a file of 8Mb.

Best wishes to @dcjones for a rapid recovery.

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