Skip to content
This repository has been archived by the owner on Jul 7, 2022. It is now read-only.

Replace TermColors class with termcolor and colorama modules #15

Merged
merged 1 commit into from Dec 13, 2017

Conversation

TheWindRider
Copy link
Contributor

@TheWindRider TheWindRider commented Dec 9, 2017

This is for issue #14
To verify the changes, I executed validate command and attached screenshot below.
aptossnap

@ghost
Copy link

ghost commented Dec 9, 2017

Your pull request doesn't follow our guidelines. Please fix the following:

  • Pull request description must include verification steps (?)

Click here for details.

Thank you! 🙏

This comment was made by GitMagic – Magically enforcing your contribution guidelines.

from .parser import SchemaParser
from .primitive import Object
from .visitor import ValidationVisitor
from .schema.visitor import AvroSchemaVisitor


''' previous color solution, not working in Windows
class TermColors:
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the TermColors class since it is not being used anymore.

@jasonwalsh
Copy link
Contributor

jasonwalsh commented Dec 11, 2017

@TheWindRider thanks for your PR! I left some feedback. Also, take a look at the build status as it appears to be failing the flake8 style linting checks.

Once the build succeeds, I'll merge it into master.

@TheWindRider
Copy link
Contributor Author

Took a closer look, and now I understand why it's failing. I forgot to replace all TermColors, and there are some additional code styling issue.

so that it works cross-platform
@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.254% when pulling e17ed2c on TheWindRider:master into d59cc23 on pennsignals:master.

@ghost
Copy link

ghost commented Dec 12, 2017

Thank you, the title and description now looks good! :bowtie:

Copy link
Contributor

@jasonwalsh jasonwalsh left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jasonwalsh jasonwalsh merged commit ed54c11 into pennsignals:master Dec 13, 2017
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.

None yet

3 participants