Skip to content

Conversation

@asamuzaK
Copy link
Contributor

@asamuzaK asamuzaK commented Dec 7, 2017

Fixes #1646

Detect file type from file extension.

The following extensions will be treated as ASCII file, other as BINARY.
[css, htm, html, js, json, mml, svg, txt, xml, xhtml]

I'm not sure that's enough or too much for linter.
Please tell me.

  • [✓] This PR relates to an existing open issue and there are no existing
    PRs open for the same issue.
  • [✓] Add Fixes #ISSUENUM at the top of your PR.
  • [✓] Add a description of the the changes introduced in this PR.
  • [✓] The change has been successfully run locally.
  • [✓] Add tests to cover the changes added in this PR.

@asamuzaK
Copy link
Contributor Author

asamuzaK commented Dec 11, 2017

Sorry, commit by mistake.
It's not related to this PR, reverted.

@rpl
Copy link
Member

rpl commented Dec 19, 2017

Hey @asamuzaK Thank you so much for opening this PR and mention it in #1714, unfortunately I did not notice that you were already working on it when I was investigating the issue in web-ext (mozilla/web-ext#1164) and so I worked on it on a different PR.

I ended up to land my PR, mostly because I preferred a more "unit test" testing strategy (which in this PR is more like an integration test, which we should definitely add to this repo, but I would prefer to add them into a separate integration test suite, instead of inside the current unit test suite) and also because of the isAscii check inside of the getFileAsStream (which could be hard to keep in sync over the time), nevertheless I kept into account your proposals from this PR and I tweaked the #1714 accordingly before landing it.

And so, long story short, even if I'm closing this PR, I would say that there is still a bit of your PR in the final #1714 version!

Thanks again!

@rpl rpl closed this Dec 19, 2017
@asamuzaK asamuzaK deleted the stream branch December 19, 2017 22:25
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