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

Added warning for arguments containing carriage returns. This happens… #1712

Merged
merged 2 commits into from
Feb 18, 2019

Conversation

Aralox
Copy link
Contributor

@Aralox Aralox commented Feb 17, 2019

… if a shell script was written in Windows but executed in Linux, as may be the case with bash for Windows.

Example of undefined behaviour:

  • sh script to invoke p4a is written in windows and run in windows bash.
  • In this script, the last argument to p4a is --requirements=python3,kivy,numpy, which causes the last package to be numpy\r due to the CRLF at the end of the line. This might get through to pip but not have any associated p4a recipes, leading to many red herrings and wild goose chases.

The solution to this was straightforward (change my text editor to use LF instad of CRLF), but this behaviour was difficult and time-consuming to debug. Although this problem is not due to python-for-android, I decided to open this PR regardless as it would improve developer quality of life.

… if a shell script was written in Windows but executed in Linux, as may be the case with bash for Windows.
AndreMiras
AndreMiras previously approved these changes Feb 17, 2019
Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

Thanks for trying to save developers time 😄
I made a comment for using format() but it's minor so merging anyway.
Maybe for later contributions you should take a look at the 50/72 formatting rule for your commits

@Aralox
Copy link
Contributor Author

Aralox commented Feb 18, 2019

Thank you for your comments Andre, and also for telling me about the 50/72 rule. I'm going to start using that at work!

Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and for addressing the comments

@AndreMiras AndreMiras merged commit d632f8e into kivy:master Feb 18, 2019
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