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

Windows fix & update #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Windows fix & update #2

wants to merge 1 commit into from

Conversation

jdarling
Copy link

Fixed so it works on windows, switched to adm-zip for decompression, and
added progress for progress monitoring when downloading Build Tools.
Also fixed a minor bug in the parser where it wasn't looking for C:
symbols and skipping them. Really it should just skip any unknown
symbols.

Fixed so it works on windows, switched to adm-zip for decompression, and
added progress for progress monitoring when downloading Build Tools.
Also fixed a minor bug in the parser where it wasn't looking for C:
symbols and skipping them.  Really it should just skip any unknown
symbols.
@rubenv
Copy link
Owner

rubenv commented Apr 19, 2014

Cool! Do you have an example of such a missing string that we can add as a test case?

@jdarling
Copy link
Author

Here is a cobbled together section, sorry I had to cut a few different dumps together to keep out company IP information:

N: android=http://schemas.android.com/apk/res/android
  E: manifest (line=2)
    A: android:versionCode(0x0101021b)=(type 0x10)0x1e
      E: service (line=73)
        A: android:exported(0x01010010)=(type 0x12)0xffffffff
        E: intent-filter (line=74)
          C: ".9\n"

That C: ".9\n" is the line I've seen. No clue what its for.

@jdarling
Copy link
Author

Oh and sorry about the catch(e)'s throwing Travis CI off, wouldn't have thought you would be using JSHint with IE mode :)

@rubenv
Copy link
Owner

rubenv commented Apr 21, 2014

IE mode?

@jdarling
Copy link
Author

The error from Travis CI is "[L141:C26] W002: Value of 'e' may be overwritten in IE." based on my try/catch blocks only using catch(e). There should be a flag on JSHint that turns off IE only warnings, sorry don't use JSHint so I don't know what the flag is, that should be safe to turn off since apk-parser really couldn't be used in a browser.

@rubenv
Copy link
Owner

rubenv commented Apr 22, 2014

I just noticed that there isn't any .jshintrc in the repo. Let me add the one I usually use.

I don't really care about IE compatibility for Node.JS code, I do care about consistent code style :-)

@rubenv
Copy link
Owner

rubenv commented Apr 22, 2014

I just added a JSHint configuration. It should cause the linting to behave more reasonably.

From looking at your diff, you'll probably get a ton of new errors and warnings, mostly about whitespace. Sorry about that, but you'll have to fix it.

The patch is currently a mix of different styles and that's just plain ugly.

For instance, there's both function(response) { (space before accolade), function(err){ (no spaces at all) and function () { (the proper way to do whitespace and anonymous functions).

I'd love to merge this (good additions!), but we need to step up the code quality a bit first.

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