-
Notifications
You must be signed in to change notification settings - Fork 131
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
Added encoding support #154
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In itself this looks like a reasonable change, but how is the user expected to use this? Are you using bump2version more as a library than as a CLI tool?
We are using bump2version as a CLI tool. If a file that is to be modified by bump2version has a non-utf8 encoding, then this has to be stated in the configuration file. E.g.:
(That way, bump2version will not raise an exception if the file is latin1 encoded and there are, for example, german umlauts in the file.) |
Could you add that to the README? With that I think this should be ok to merge. Ideally it would also have a test. |
I've added documentation to the README and a test. |
The test should also verify that the file content is still intact in the correct encoding. I'll take a look. |
Hi 4hopp, I extended the test a bit. If you approve that merge in your branch (next time, create a separate feature branch for it!), then this can go in. |
Hi there,
please add encoding support for files to be modified. This feature is very important to us as we maintain many files that use to have latin1 encoding for legacy reasons.
(An alternative solution might be to read files with binary mode; I have not tested this, though, and there might be sideeffects in doing so.)
Cheers,
Joshua Hopp