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

Feature/windows #441

Merged
merged 3 commits into from
Apr 5, 2020
Merged

Feature/windows #441

merged 3 commits into from
Apr 5, 2020

Conversation

rachmadaniHaryono
Copy link
Collaborator

  • use general Exception instead ImportError+ModuleNotFoundError, imo more expected result than adding ModuleNotFoundError to python3.5
  • package requirement is taken from https://www.python.org/dev/peps/pep-0508/

i put do-not-merge label because i don't have windows dev to test it. the pr is actually finished except if there is another commit needed to fix problem from windows tester

@jarun
Copy link
Owner

jarun commented Mar 29, 2020

@dertuxmalwieder would it be possible for you to confirm this?

@dertuxmalwieder
Copy link
Contributor

According to the code, it should only behave differently on non-Windows platforms now...?

@jarun
Copy link
Owner

jarun commented Mar 29, 2020

I think the only difference is it adds pyreadline as an additional dep on Windows.

@rachmadaniHaryono is pyreadline availabe by default on other platforms? Otherwise can we add it as a dep on all platforms?

@rachmadaniHaryono
Copy link
Collaborator Author

According to the code, it should only behave differently on non-Windows platforms now...?

yes, except if there is something wrong with what i write on general exception

I think the only difference is it adds pyreadline as an additional dep on Windows.

and also general exception on readline import error

@rachmadaniHaryono is pyreadline availabe by default on other platforms? Otherwise can we add it as a dep on all platforms?

i don't think so. based on pyreadline readme, it is actually only tested on windows 2000 and windows xp

The pyreadline package is a python implementation of GNU readline functionality it is based on the ctypes based UNC readline package by Gary Bishop. It is not complete. It has been tested for use with windows 2000 and windows xp.

@jarun
Copy link
Owner

jarun commented Mar 29, 2020

It has been tested for use with windows 2000 and windows xp.

In that case, the change looks fine to me,

@jarun
Copy link
Owner

jarun commented Apr 3, 2020

@dertuxmalwieder any update on this?

@dertuxmalwieder
Copy link
Contributor

I haven’t tried my theory yet. ;-)
LGTM

@jarun
Copy link
Owner

jarun commented Apr 5, 2020

@rachmadaniHaryono can we merge this? (labeled do-not-merge)

@rachmadaniHaryono
Copy link
Collaborator Author

yes, because @dertuxmalwieder said ok, i think it can be merged.

any related error on windows user can be fixed on another pr.

i will leave the label as it is. @jarun, you can change it if you want

@jarun jarun removed the do-not-merge label Apr 5, 2020
@jarun jarun merged commit 9fce289 into jarun:master Apr 5, 2020
@jarun
Copy link
Owner

jarun commented Apr 5, 2020

Thank you guys!

@github-actions github-actions bot locked and limited conversation to collaborators Jun 15, 2020
@rachmadaniHaryono rachmadaniHaryono deleted the feature/windows branch November 12, 2020 23:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants