search

aadsm / jschardet

Character encoding auto-detection in JavaScript (port of python's chardet)
GNU Lesser General Public License v2.1
714 stars 97 forks source link

Corrected "TypeError: Cannot assign to read only property", closes #68 #75

Closed danielgindi closed 2 years ago

maiermic commented 2 years ago

@danielgindi Thanks for creating a PR. How did you test it? I saw that there is a merged PR in a forked repository: https://github.com/alist-org/jschardet/pull/1. Has this been used for testing?

danielgindi commented 2 years ago

I've tested it in a production system with a few inputs that failed due to this error, and got detected correctly with this PR. What I did is simply correct the code syntax to do what the author seems to have intended.

maiermic commented 2 years ago

@danielgindi Thanks for your response. How did you include your changed version of jschardet in your project? I installed jschardet using npm in my project, but AFAIK that does not work without a release of this PR.

@aadsm Does an example project (with the fixed version of jschardet) help you to review the PR? Are manual changes to the codebase even the way to go? Or is this codebase still/regularly generated/ported based on the Python codebase?

danielgindi commented 2 years ago

@danielgindi Thanks for your response. How did you include your changed version of jschardet in your project? I installed jschardet using npm in my project, but AFAIK that does not work without a release of this PR.

Just put the git url of the PR branch as the package version.

aadsm commented 2 years ago

Let me see if I can check this tomorrow, otherwise on Wednesday where I'll have more time. If I don't please ping me again.

maiermic commented 2 years ago

If I don't please ping me again.

@aadsm Friendly reminder. Don't hurry. Take your time. We are all busy :wink: Thanks for creating and maintaining this project :relaxed:

aadsm commented 2 years ago

@aadsm Does an example project (with the fixed version of jschardet) help you to review the PR? Are manual changes to the codebase even the way to go? Or is this codebase still/regularly generated/ported based on the Python codebase?

If you could add a test here that would be great. Ignore the qunit stuff, that's what I used at the time, but I've just ported them to jest.

I only (manually) converted this from python when I created the project 12 years ago.

aadsm commented 2 years ago

I've just checked the source line with the issue. I guess this is what I get for not using type checkers, although at the time none existed for javascript. Thank you so much for fixing this! I'm going to merge but could someone still add a unit test pretty please? 😇.

vibaiher-qatium commented 2 years ago

Hello @aadsm

Do you plan to publish this change in a new version soon?