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

match d43, d65, etc. #1363

Merged
merged 3 commits into from
Aug 10, 2022
Merged

Conversation

malcolmsailor
Copy link
Contributor

I went ahead and implemented a regex to make other inversions prefaced by "d" also dominant seventh chords as I suggested in #1358. It was just a couple lines of code and it will allow me to clean up some of the stray chords still not matching with the tsvConverter.

NB pylint objects music21/roman.py:2378: [R0201(no-self-use), RomanNumeral._findSemitoneSizeForQuality] Method could be a function but this is not code introduced by me.

My remaining question is: perhaps 'd' should indicate dominant seventh quality no matter what digits follow? I.e., if a user indicates something weird like "d479', we should just pass the digits straight through but set impliedQuality to 'dominant-seventh'?

@coveralls
Copy link

coveralls commented Aug 8, 2022

Coverage Status

Coverage decreased (-0.0009%) to 93.076% when pulling 41f264a on malcolmsailor:dominant_7ths into c2f5c6d on cuthbertLab:master.

@mscuthbert
Copy link
Member

NB pylint objects music21/roman.py:2378: [R0201(no-self-use), RomanNumeral._findSemitoneSizeForQuality] Method could be a function but this is not code introduced by me.

We have no-self-use disabled in the music21 pylint configuration. Make sure that pylint is reading the file.

Copy link
Member

@mscuthbert mscuthbert left a comment

Choose a reason for hiding this comment

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

Great! Some small and fast changes then ready to go. Thanks!

music21/roman.py Outdated Show resolved Hide resolved
music21/roman.py Show resolved Hide resolved
music21/roman.py Outdated Show resolved Hide resolved
@malcolmsailor
Copy link
Contributor Author

We have no-self-use disabled in the music21 pylint configuration. Make sure that pylint is reading the file.

I don't see it in the .pylintrc in the repository root directory. Is there more pylint configuration elsewhere?

@mscuthbert
Copy link
Member

We have no-self-use disabled in the music21 pylint configuration. Make sure that pylint is reading the file.

I don't see it in the .pylintrc in the repository root directory. Is there more pylint configuration elsewhere?

Ah! For various reasons, we have to put the configuration in twice -- once in music21/test/testLint.py and once in the .pylintrc -- they're supposed to be kept perfectly in sync, but obviously they weren't for this point.

@mscuthbert
Copy link
Member

Looking good. Letting tests run (I should've had you do a one-byte PR to get you approved as not "first-time-contributor" long ago), and if all pass will merge. THANKS!

@malcolmsailor
Copy link
Contributor Author

Looking good. Letting tests run (I should've had you do a one-byte PR to get you approved as not "first-time-contributor" long ago), and if all pass will merge. THANKS!

As you can see the tests are failing with the same error I referenced in #1267; any suggestions for what might be going on with that are welcome.

@mscuthbert
Copy link
Member

It's the 3.10.6 change that was causing the failure. Fix pushed and running tests again.

@mscuthbert mscuthbert closed this Aug 10, 2022
@mscuthbert mscuthbert reopened this Aug 10, 2022
@mscuthbert
Copy link
Member

We have no-self-use disabled in the music21 pylint configuration. Make sure that pylint is reading the file.

I don't see it in the .pylintrc in the repository root directory. Is there more pylint configuration elsewhere?

Ah -- no-self-use was moved to an optional extension in recent versions of pylint, and is no longer checked. So the solution is to upgrade your pylint.

@mscuthbert mscuthbert merged commit 6f684b3 into cuthbertLab:master Aug 10, 2022
@mscuthbert
Copy link
Member

Merged. Congrats!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants