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

add missing methods which are causing issues in haxe.zip.Reader #160

Merged
merged 3 commits into from
Apr 6, 2020

Conversation

peteshand
Copy link
Contributor

No description provided.

@Gama11
Copy link
Member

Gama11 commented Apr 6, 2020

Does this really help with anything? Looks like it will just lead to a runtime error as soon as you call the constructor.

@peteshand
Copy link
Contributor Author

Yeah it's stops a compile error.

In the std lib, haxe.zip.Reader calls
new Uncompress, which throws an error. This bug was reported quite a while ago, so would be good to resolve it

@Gama11
Copy link
Member

Gama11 commented Apr 6, 2020

But it doesn't look like this actually solves anything, it just moves the error from compile time to runtime?

@peteshand
Copy link
Contributor Author

peteshand commented Apr 6, 2020

Hmm that's not entirely true, It moves the error from always happening at compile time, to only happening when you call a particular method on Reader. For example I'm not using Reader, but because of this in compatibility my whole app won't compile. And if you are using Reader then your no worse off, as your app already won't be compiling.

@Gama11
Copy link
Member

Gama11 commented Apr 6, 2020

Hm, I see... Do you have a code example?

@peteshand
Copy link
Contributor Author

peteshand commented Apr 6, 2020

It's GMT+10 so not at my computer. But basically hxnodejs overrides a std class and removes a bunch of methods, this is going to cause issues, shouldn't need a code example to see this. Could you just merge please

@Gama11
Copy link
Member

Gama11 commented Apr 6, 2020

Calm down. :) It's not about understanding, I was thinking it would make for a good test case for unit tests / CI. But it looks like I misremembered and we don't actually have unit tests for hxnodejs, just these examples.

@nadako
Copy link
Member

nadako commented Apr 6, 2020

It's probably fine to merge this, tho for Uncompress you can probably use this code.

@nadako nadako merged commit af30e67 into HaxeFoundation:master Apr 6, 2020
@peteshand
Copy link
Contributor Author

Thanks

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