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

Fix midtrans-python-client to support windows environment #12

Merged
merged 3 commits into from
Oct 25, 2021

Conversation

hyuma
Copy link

@hyuma hyuma commented Feb 4, 2020

To support Windows.
Without this fix, UnicodeDecodeError: 'cp932' codec can't decode byte 0xef in position 515: illegal multibyte sequence will occor when pip install

@hyuma hyuma requested a review from rizdaprasetya February 4, 2020 04:44
@hyuma
Copy link
Author

hyuma commented Feb 4, 2020

(this is bugfix)

@rizdaprasetya
Copy link
Collaborator

Nice, thanks for the contribution! @hyuma
But according to our check few months ago it will break compatibility with Python 2.7
rizdaprasetya#3

The best way to fix Windows compatibility and retain Python 2.7 compatibility is by following this sample setup.py file:
https://github.com/pypa/sampleproject/blob/master/setup.py#L8-L24

Would you like to try to add that?

Copy link
Collaborator

@rizdaprasetya rizdaprasetya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow: #12 (comment)

@mashanz
Copy link
Contributor

mashanz commented Oct 21, 2021

Hi @rizdaprasetya , should we just make release version/branch/tag for backward compatibility?

  1. let say, release the un merged branch with version/branch/tag 1.0.0 (compatibility with python 2)
  2. and merge the branch and release it with version/branch/tag 2.0.0 and throw everything about python 2 compatibility
  3. with this way, it will made easier to track version and compatibility like new version of python or new version of dependencies

also the other reason is python 2 already dead almost 2 years ago. though?

@rizdaprasetya
Copy link
Collaborator

@mashanz correct, thanks for notifying, sorry we haven't review this for a while. Agree with your suggestion, esp. since python 2 is no longer supported officially. we'll use better version tagging to handle such compatibility concerns.

Also if you see any more compatibility issue to latest/current python version feel free to let us know by opening issue/PR. Cheers!

- for better compatibility across OS (esp Windows) & latest official python version (Python 3)
- may not work on Python 2 anymore
Copy link
Collaborator

@rizdaprasetya rizdaprasetya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the setup.py implementation to follow official pypa sample project. It should have better compatibility with Windows OS & Python 3 now.

Thanks for your contribution @hyuma & @mashanz! 😃
Will merge now, and release as new version soon. Thanks

@rizdaprasetya rizdaprasetya merged commit 5f91625 into Midtrans:master Oct 25, 2021
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