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

bootstrap: Fix all the pep-8 issues reported by flake8 #43003

Merged
merged 2 commits into from
Jul 2, 2017
Merged

bootstrap: Fix all the pep-8 issues reported by flake8 #43003

merged 2 commits into from
Jul 2, 2017

Conversation

milmazz
Copy link
Contributor

@milmazz milmazz commented Jul 1, 2017

This commit also adds a few missing docstrings.

Today, after reading this article, I downloaded this project and started building from source. In the meantime, I began to read the bootstrap.py, to know more about the building process, and I made a few changes, this is my first contribution to the project, hope you like it.

BTW, I have a few doubts about the bootstrap.py, any guidance is more than welcome:

  • Where can I find the unit tests for this script? In case it doesn't exist yet, do you like to include some unit tests with pytest?
  • Some methods like fix_executable, get_string, and exe_suffix in the RustBuild class should be converted to a function because it doesn't use self anywhere. What do you think?

This commit also adds a few missing docstrings
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@zackmdavis
Copy link
Member

(not a core reviewer, but commenting anyway because Python!!)

do you like to include some unit tests with pytest?

If there were to be unit tests, I'd imagine they should be with standard-libary unittest rather than pytest. (Python's batteries-included philosophy is part of what makes it such a good scripting-glue language; it's better if Rust's build process doesn't depend on getting a third-party library from PyPI.)

should be converted to a function because it doesn't use self anywhere

Or @staticmethod?

@Mark-Simulacrum
Copy link
Member

These changes primarily look good to me. I'll r? @alexcrichton for approval though.

Where can I find the unit tests for this script? In case it doesn't exist yet, do you like to include some unit tests with pytest?

I don't know if there's much in bootstrap.py that really could be unit tested -- most of it is setup code interacting with the network or file system in preparation for handing off to rustbuild itself. I'm not opposed to adding tests for the parts we can test, though!

Some methods like fix_executable, get_string, and exe_suffix in the RustBuild class should be converted to a function because it doesn't use self anywhere. What do you think?

Seems like a good idea, though not familiar with python overly much myself. Feel free to add that here if you'd like!

@Mark-Simulacrum Mark-Simulacrum assigned alexcrichton and unassigned brson Jul 2, 2017
@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 2, 2017
@alexcrichton
Copy link
Member

@bors: r+

We always welcome refactorings of our Python, as you can probably tell we in general aren't the most seasoned Python programmers, so help is always very much appreciated!

@bors
Copy link
Contributor

bors commented Jul 2, 2017

📌 Commit 44c6781 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Jul 2, 2017

⌛ Testing commit 44c6781 with merge c3a130c...

bors added a commit that referenced this pull request Jul 2, 2017
bootstrap: Fix all the pep-8 issues reported by flake8

This commit also adds a few missing docstrings.

Today, after reading this [article](https://blog.rust-lang.org/2017/06/27/Increasing-Rusts-Reach.html), I downloaded this project and started building from source. In the meantime, I began to read the `bootstrap.py`, to know more about the building process, and I made a few changes, this is my first contribution to the project, hope you like it.

BTW, I have a few doubts about the `bootstrap.py`, any guidance is more than welcome:

* Where can I find the unit tests for this script? In case it doesn't exist yet, do you like to include some unit tests with pytest?
* Some methods like `fix_executable`, `get_string`, and `exe_suffix` in the `RustBuild` class should be converted to a function because it doesn't use `self` anywhere. What do you think?
@bors
Copy link
Contributor

bors commented Jul 2, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing c3a130c to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants