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

In song lists augment sort tag for tag patterns with conditional tag … #1783

Merged
merged 5 commits into from Jan 14, 2016

Conversation

Projects
None yet
2 participants
@pfps
Contributor

pfps commented Jan 13, 2016

…pattern that selects sort version of tag if present.

All that this does is replace any that has a sort version with <sorttag||> when building the sort tag from a tag pattern.

In song lists augment sort tag for tag patterns with conditional tag …
…pattern that selects sort version of tag if present
@lazka

This comment has been minimized.

Member

lazka commented Jan 14, 2016

Looks good. Can you fix the travis tests? https://travis-ci.org/quodlibet/quodlibet/jobs/102230304#L1067

@pfps

This comment has been minimized.

Contributor

pfps commented Jan 14, 2016

It appears that the problem was a line longer than 79 characters. I
reformatted the line, and uploaded the change.

peter

PS: I wonder why there is a check for lines less than 80 characters.

On 01/14/2016 04:17 AM, Christoph Reiter wrote:

Looks good. Can you fix the travis tests?
https://travis-ci.org/quodlibet/quodlibet/jobs/102230304#L1067


Reply to this email directly or view it on GitHub
#1783 (comment).

@lazka

This comment has been minimized.

Member

lazka commented Jan 14, 2016

PS: I wonder why there is a check for lines less than 80 characters.

We run pep8 and pyflakes, both with some errors and warnings disabled/ignored which we found too strict/annoying. It makes sure that the whole code base remains readable and has a consistent style. The 80 char limit is mostly there because pep8 has it by default and we haven't had the need to disabled/change it. If you prefer a different limit feel free to open a bug report about it.

You can invoke the tests yourself locally using "./setup.py quality". See https://quodlibet.readthedocs.org/en/latest/development/testing.html#testing

@pfps

This comment has been minimized.

Contributor

pfps commented Jan 14, 2016

I was just wondering about the line limit. I'm not going to agitate to change it.

I tried to run the tests locally, but pep8 was not installed. It took some digging to find it. And then the output is strange

idefix quodlibet> ./setup.py quality
running quality
TPEP8 (1):                E                                                   0
================================================================================
ERROR: test_all (tests.quality.test_pep8.TPEP8)
--------------------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/local/SoftwareDownloads/quodlibet-fork/quodlibet/tests/quality/test_pep8.py", line 86, in test_all
    raise Exception("\\n".join(errors))
Exception: 

[lots of blank lines]

TPyFlakes (1):            +                                                   1
0 test failure(s) and 1 test error(s), as detailed above.
idefix quodlibet> 

This is on an up-to-date Fedora distribution.

Anyway, not a severe problem, it just means that I have to push and look at the builds on github for this.

@pfps

This comment has been minimized.

Contributor

pfps commented Jan 14, 2016

Aah, you have to
dnf install python-pep8
not
pip install pep8
and then pep8 works OK on Fedora.

lazka added a commit that referenced this pull request Jan 14, 2016

Merge pull request #1783 from pfps/master
In song lists augment sort tag for tag patterns with conditional tag …

@lazka lazka merged commit 2088fd1 into quodlibet:master Jan 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lazka

This comment has been minimized.

Member

lazka commented Jan 14, 2016

Thanks

@pfps

This comment has been minimized.

Contributor

pfps commented Jan 14, 2016

Great!

By the way, how easy was it to set up the automatic checks? Is there a cookbook for doing it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment