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

Write roman ts and repeats #1393

Merged
merged 17 commits into from
Oct 7, 2022

Conversation

MarkGotham
Copy link
Contributor

Write complement to #1378 along with a bit of clean up

@coveralls
Copy link

coveralls commented Aug 17, 2022

Coverage Status

Coverage decreased (-0.0003%) to 93.036% when pulling a25cdf5 on MarkGotham:writeRoman_ts_and_reapeats into 5417b3c on cuthbertLab:master.

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.

Please see comments on removing tested/documented code without replacements.

music21/romanText/writeRoman.py Outdated Show resolved Hide resolved
music21/romanText/writeRoman.py Outdated Show resolved Hide resolved
music21/romanText/writeRoman.py Show resolved Hide resolved
music21/romanText/writeRoman.py Outdated Show resolved Hide resolved
music21/romanText/writeRoman.py Outdated Show resolved Hide resolved
music21/romanText/writeRoman.py Outdated Show resolved Hide resolved
music21/romanText/writeRoman.py Outdated Show resolved Hide resolved
@MarkGotham
Copy link
Contributor Author

Ok, noted.

The mass deletions are an attempt at simpler solutions and strike me as a good fit for the contributing docs' invitation to remove 30+ lines when 1 will do nicely. All the same, while I have misgiving about rnString (for that reason among others), I'll go ahead and reinstate it with the minimum changes to accommodate new tokens (i.e., repeat marks).

For reference, the process is about the addition of specific tokens in a specific order.

  • measure (required)
  • (optional) start repeat
  • RNs (with beat except for b1, at least one required)
  • (optional) end repeat

@MarkGotham
Copy link
Contributor Author

This PR has been superseded by another, subsequent one for the same issue at #1435.

I wonder if there's an integration action for checking new PRs against those in progress that would prevent this?

In any case, I've been through to pick them apart and what remains as new here is simply:

  • adding a doc test and
  • one case of specifying insert() at position 0 (as requested)
  • misc clean up and clarification

A small contributions for sure, but a positive one, and makes this (1393) ready for closure.

Let me know if you spot any issues / anything missing.

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.

The contributions here are enough to still merge after the changes requested are made.

If you know of an easy tool that can prevent duplicated work, happy to add it.

music21/romanText/writeRoman.py Outdated Show resolved Hide resolved
music21/romanText/writeRoman.py Show resolved Hide resolved
music21/romanText/writeRoman.py Show resolved Hide resolved
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.

LGTM. Thanks!

@mscuthbert mscuthbert merged commit 21f3df5 into cuthbertLab:master Oct 7, 2022
@MarkGotham MarkGotham deleted the writeRoman_ts_and_reapeats branch October 7, 2022 07:54
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