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

replacing "31" with RED constant #381

Closed
wants to merge 2 commits into from
Closed

replacing "31" with RED constant #381

wants to merge 2 commits into from

Conversation

@d-jh
Copy link

d-jh commented May 22, 2016

This change is Reviewable

@emilio
Copy link
Member

emilio commented May 22, 2016

This looks good to me, but it seems that travis complains:

./tests/lint/trailing_whitespace.py:11:18: F821 undefined name 'RED'

You should import the constant it seems.

@aneeshusa
Copy link
Member

aneeshusa commented May 24, 2016

@d-jh Thanks for the PR! @emilio is correct that you need to import the constant. Instead of using * to import everything, please add RED to the list of imports instead. Using the glob-style import makes it impossible to know what you're importing from where, which causes problems over time as files get bigger and definitions move around.

@jdm
Copy link
Member

jdm commented Jun 8, 2016

@d-jh Are you planning to finish this up?

@KiChjang
Copy link
Member

KiChjang commented Jun 17, 2016

Closing due to lack of reply for 23 days.

@KiChjang KiChjang closed this Jun 17, 2016
bors-servo added a commit that referenced this pull request Jun 18, 2016
Replaced hard coded color with RED constant from utils.py

This is to fix: #378

#381 had been closed due to inactivity so I picked this task to start contributing to servo. Hopefully this resolves the outstanding comments from 381.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/400)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.