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

Added Dorico Support #1187

Closed
wants to merge 1 commit into from
Closed

Conversation

EvilArchitech
Copy link

No description provided.

@jacobtylerwalls jacobtylerwalls linked an issue Dec 18, 2021 that may be closed by this pull request
Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Thanks for moving this forward! I'll approve the CI run once you can push changes -- I'd like to be sure the job won't hang because of the change to TestUserInput.

Comment on lines -1599 to +1610
class TestUserInput(unittest.TestCase): # pragma: no cover
class TestExternal(unittest.TestCase): # pragma: no cover
Copy link
Member

Choose a reason for hiding this comment

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

This is not named TestExternal so that it won't run on CI, where we would have to patch the interactive user inputs so that the job doesn't hang.

Copy link
Author

Choose a reason for hiding this comment

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

I don't understand the comment.

Copy link
Member

Choose a reason for hiding this comment

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

The class must not be called TestExternal or it will run and hang our automated testing system.

@@ -1762,15 +1773,15 @@ def run():
else:
# only if running tests
t = Test()
te = TestUserInput()
te = TestExternal()
Copy link
Member

Choose a reason for hiding this comment

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

sim.

Copy link
Author

Choose a reason for hiding this comment

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

sim, I don't understand the comment. :)

Comment on lines -1773 to +1784
# run test user input
# run test external
Copy link
Member

Choose a reason for hiding this comment

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

I suppose this shouldn't be 'te' anymore.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I don't understand the comment.

Comment on lines -48 to +50
urlMusic21 = 'https://web.mit.edu/music21'
urlMusic21 = 'http://web.mit.edu/music21'
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious why this unrelated change? Sphinx linkcheck will emit errors for redirects from http -> https, so we just link to https.

Copy link
Author

Choose a reason for hiding this comment

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

I did not (Intentionally) make any such change.

Comment on lines -50 to +53
urlGettingStarted = 'https://web.mit.edu/music21/doc/'
urlMusic21List = 'https://groups.google.com/g/music21list'
urlGettingStarted = 'http://web.mit.edu/music21/doc/'
urlMusic21List = 'http://groups.google.com/group/music21list'
Copy link
Member

Choose a reason for hiding this comment

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

sim., I think Sphinx will complain if we make a change.

Comment on lines -1482 to +1494
msg.append('Welcome to the music21 Configuration Assistant. You will be guided '
+ 'through a number of questions to install and set up music21. '
msg.append('Welcome the music21 Configuration Assistant. You will be guided '
+ 'through a number of questions to install and setup music21. '
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious why this unrelated change? Also "set up" is the correct verb phrase here; "setup" is a noun.

Copy link
Author

Choose a reason for hiding this comment

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

I did not (intentionally) make any such change.

@jacobtylerwalls
Copy link
Member

@EvilArchitech Hey, I think I figured out what happened here. It's likely you began by editing an outdated version of this file, perhaps from version 6.7, and then uploaded it to GitHub. Instead you'll need to base your changes on an updated version of music21 based on the master branch. You can download it from GitHub and re-upload it if you're more comfortable with uploading than with doing a git clone.

@EvilArchitech
Copy link
Author

Got it, I will take a look at the master. I did the upload only because I have an SSO linked corporate login to GitHub, and it was a pain to switch logins without messing up other repos in MS Code.

@mscuthbert
Copy link
Member

I think it'd be easier to close the PR and then open a new one from when you're sure you're on the current music21 master.

@mscuthbert mscuthbert closed this Jan 12, 2022
@mscuthbert
Copy link
Member

Closing so there's no danger of merging. Feel free to open a new PR based on latest master.

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.

Add Dorico as a potential Music XML Reader
3 participants