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

Add qsscheck to CI pipeline and improve .travis.yml config #2375

Merged
merged 20 commits into from
Dec 9, 2019

Conversation

Holzhaus
Copy link
Member

This adds a simple checker script for *.qss files to the scripts/ folder and runs it on Travis CI builds. It also cleans up the .travis.yml file a bit.

@uklotzde
Copy link
Contributor

Please check for trailing whitespaces that CodeFactor complains about.

@uklotzde uklotzde added this to the 2.3.0 milestone Nov 29, 2019
@Holzhaus
Copy link
Member Author

Please check for trailing whitespaces that CodeFactor complains about.

Ok, done.

@Holzhaus
Copy link
Member Author

By the way, the qsscheck job currently fails, but the issues are fixed by PRs #2374 and #2342.

@ywwg
Copy link
Member

ywwg commented Nov 29, 2019

checking by travis is good, but is there a way we can integrate it into mixxx-test? even just having a simple C++ test file that shells out to python could work.

Copy link
Member

@ywwg ywwg left a comment

Choose a reason for hiding this comment

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

Just some python style nits. Thanks for this useful utility!

scripts/qsscheck.py Outdated Show resolved Hide resolved
scripts/qsscheck.py Outdated Show resolved Hide resolved
scripts/qsscheck.py Outdated Show resolved Hide resolved
for root, dirs, fnames in os.walk(os.path.join(mixxx_path, 'src')):
for fname in fnames:
ext = os.path.splitext(fname)[1]
if ext in ('.h', '.cpp'):
Copy link
Member

Choose a reason for hiding this comment

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

(see below comment -- here's another place where we can save a level of indentation)

Copy link
Member Author

Choose a reason for hiding this comment

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

How? We have an elseif case, too.

@Holzhaus
Copy link
Member Author

@ywwg: checking by travis is good, but is there a way we can integrate it into mixxx-test? even just having a simple C++ test file that shells out to python could work.

What's the benefit? For PRs that only make skin changes, you'd have to wait until Mixxx is built (2 minutes vs >30min). Also, this would also introduce a dependency on Python/PyQt/tidycss, which means that we need to install them on OSX and Appveyor, too. That would increase the likelyness of running into a timeout.

@ywwg
Copy link
Member

ywwg commented Nov 30, 2019

introduce a dependency on Python/PyQt/tidycss

If people want to run the script they will need these things anyway, right?

But I think it's ok to submit without having test integration. But we should make sure to update the skinning documentation to call out this script so people know about it.

@Holzhaus
Copy link
Member Author

Holzhaus commented Dec 2, 2019

I'll update the wiki after merge (since the tool is not in the scripts directory without this PR).

Is there something missing or can this be merged as soon as #2374 and #2342 have landed in master?

@Holzhaus Holzhaus added the build label Dec 5, 2019
@Holzhaus
Copy link
Member Author

Holzhaus commented Dec 8, 2019

Let's wait for the CI builds. If we have a clean state and nobody raises any concerns, I'll merge this tomorrow, so that this can prevent introducing new issues in the other pending skin PRs.

@Holzhaus Holzhaus merged commit 1456b8b into mixxxdj:master Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants