Skip to content

[WIP] GZIP Support#63

Open
AbhinavChede wants to merge 29 commits intoqiyunlab:masterfrom
AbhinavChede:gzip
Open

[WIP] GZIP Support#63
AbhinavChede wants to merge 29 commits intoqiyunlab:masterfrom
AbhinavChede:gzip

Conversation

@AbhinavChede
Copy link
Collaborator

Hi @qiyunzhu ,

I have added the functionality for BinaRena to read gzip files. For some reason, the min file of pako.es5 was not being read so ended up using the normal pako.es5 file.

Thank you
Abhinav Chede

Copy link
Collaborator

@qiyunzhu qiyunzhu left a comment

Choose a reason for hiding this comment

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

@AbhinavChede Good job! I am looking forward to testing it after you resolve my minor comment.

I have updated the coding standard to later JavaScript (ES6 and above). Therefore you can use the non-es5 version of pako.

}

// parse as gzip text
else if(text.length > 3 && text.charCodeAt() === 31 && text.charCodeAt(1) === 65533 && text.charCodeAt(2) === 8) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the standard way of identifying a gzipped file? It will be good to add a link to the source in the docstring. Here is an example.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also add a space after else if.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also make the line no longer than 80 characters, like:

else if (text.length > 3 && text.charCodeAt() === 31 && text.charCodeAt(1) === 65533 &&
         text.charCodeAt(2) === 8) {

@AbhinavChede
Copy link
Collaborator Author

Sorry for the delay on this pull request. I have made the requested syntactical changes. Also, I actually did not find any actual documentation regarding how to identify GZIP files in JS. I was looking in StackOverFlow and found out that is how to identify GZIP files.

Copy link
Collaborator

@qiyunzhu qiyunzhu left a comment

Choose a reason for hiding this comment

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

This PR appears to be mixed with some outdated code. In the "Files changed" tab, you see that <<<<<< thing, which is a git conflict. The code other than gzip-related code are the outdated ones. If we merge, this old code will replace the new one. You may first resolve the conflict, then try git pull upstream master to see if the old code can be replaced by new ones. But I am not sure.

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