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 encoding to utfbom reader structure #9

Merged

Conversation

peczenyj
Copy link
Contributor

Hello

I note that we may lost the original encoding if we wrap the *utfbom.Reader again.

This will ensure that we will return the original encoding instead Unknown

Also, I made some modifications to code and tests to ensure better readability. for instance, now all tests are running in parallel, all tests may have subtests, we stop use deprecated libraries such as io/ioutil, etc.

I also add one basic Makefile to help run tests, etc.

And drop support/ tests for old versions of go -- we are in the 1.21.1 right now

Regards

@peczenyj
Copy link
Contributor Author

Fixes #10 , #11 and #12

@dimchansky dimchansky merged commit f739413 into dimchansky:master Nov 20, 2023
@dimchansky
Copy link
Owner

Hi @peczenyj,

First off, thank you so much for your contribution to the project! I've reviewed the pull request and have merged it. However, I noticed that the Makefile uses utilities like golint, gofumpt, and goimports. While these are essential tools, they might not be present on every developer's machine.

To streamline our development process, I suggest we add an additional step in the Makefile for installing these necessary utilities. This step will ensure that the right versions of the utilities are installed, making our setup more robust and developer-friendly.

Additionally, I agree that having a Makefile is beneficial for the project. Considering the transition from Travis CI to GitHub Actions, the Makefile will play a crucial role in simplifying and automating our build process. This move aligns well with our goals for a more efficient and modern development workflow.

Looking forward to your thoughts on this, and thanks again for your valuable contribution!

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