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

Mountain Dulcimer Updates #7033

Merged
merged 1 commit into from
Dec 19, 2020
Merged

Mountain Dulcimer Updates #7033

merged 1 commit into from
Dec 19, 2020

Conversation

JeffRocchio
Copy link

Extended the fret and pitch ranges of the three Mountain Dulcimer instruments
to conform with what are now the common ranges for modern chromatic dulcimers.

Relates back to, as a refinement: https://musescore.org/en/node/153016

A luthier pointed out to me that today's chromatic mountain dulcimer builds routinely extend the fret and pitch range a bit further down the fretboard. So I am here proposing updates to the three Mountain Dulcimer instrument profiles in instruments.xml.

This is my first pull request attempt, so please do point out any missteps I may have taken here. Also, it was unclear which branch I should go up against - the 3.x branch seems to be what the vast majority of pull requests are hitting so that is what I have used here.

  • I signed CLA
  • I made sure the code in the PR follows the coding rules
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code
  • I made sure the title of the PR reflects the core meaning of the issue you are solving
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
  • I made sure the commit message title starts with "fix #424242:" if there is a related issue
  • I created the test (mtest, vtest, script test) to verify the changes I made

@njvdberg
Copy link
Contributor

njvdberg commented Dec 6, 2020

The mtest violations seems to come from a not update reference file, not to worry too much (yet? 😄) See also PR #7032.
Try to push again after PR #7032 has been merged, then the violation should be gone.

@JeffRocchio
Copy link
Author

The mtest violations seems to come from a not update reference file, not to worry too much (yet? smile) See also PR #7032.
Try to push again after PR #7032 has been merged, then the violation should be gone.

Thanks Niek, I tried to work out was causing that but wasn't able to. It seemed unrelated to my changes so I was hoping someone would weigh in. I'll keep a watch on PR 7032 as you suggest.

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Dec 8, 2020

Bad merge. Use git pull --rebase musescore 3.x if 'musescore' is the name of your upstream repo), then git push -f
The mtest fails are now due to a different reason, but again not your fault...

@JeffRocchio
Copy link
Author

I did the rebase as Jojo suggested; and my gitHub fork, and local, shows they are in sync with musescore 3.x upstream. But still the tests fail.

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Dec 9, 2020

No, apparently you didn't, you're having many (20 out of 21) commits in your branch that don't belong there.

The vtests and mtests are known to be currently broken though, once again

@JeffRocchio
Copy link
Author

JeffRocchio commented Dec 9, 2020

...you're having many (20 out of 21) commits in your branch...

Yikes! I wish I could understand why that happened. I have touched only the one single file: instruments.xml.

Here's what I did: 1. I copy/pasted your first command into my terminal. It came back and said there was no such repository. 2. I then spent an hour or so studying the rebase command documentation & some tutorials. 3. Based on that, and given that when I init'd my local git I did set the upstream URL to the musescore repo, I executed: [git pull --rebase upstream 3.x]. 4. I then executed [git pull] to pull the merged updates down to my local machine. Then I executed [git push -f]. 5. Git gave me messages that I interpreted as 'you're up to date with the upstream repo, and you have 1 unmerged commit.' 6. I then looked at this pull request on musecore and it had initiated the tests.

Now, before I did all the above - on Dec-8 before Jojo made his recommendation to me, I used the [git commit --amend] command in an attempt to re-push my pull request without touching the instruments.xml file (I didn't want to do something like add a spurious space to make the file look to git like it had been edited). So perhaps this is what has messed the pull request up (beyond hope)?

I fully confess that, having come out of the corporate banking world, I have no practical experience of using git on a large, highly, collaborative project such as this. I am making a good-faith attempt to learn, but haven't, so far, found any good tutorials on the particulars of using gitHub, a fork and a local repo in concert on a large project where deviations from the 'happy path' naturally occur. I have been referencing the Musescore Git Workflow page tho.

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Dec 9, 2020

Step 2 was correct, git pull --rebase upstream 3.x (for me 'musescore'is the name of the upstream repo).
Step 3 was wrong, git pull, at least not needed (and not mentioned by me). Likely the cause for the mess.
Step 4 was correct, git push -f.

That git commit --amend was not needed, but won't harm (if followed by anoher git push -f). It won't rebase though.

So just do step 2 and 4 again.

@JeffRocchio
Copy link
Author

So just do step 2 and 4 again.

Ok, got it. Thanks Jojo.

@JeffRocchio
Copy link
Author

"Cancelled after 18m" ?? Now what's that about? I'm beginning to think that GitHub simply doesn't want my change.

@Jojo-Schmitz
Copy link
Contributor

Just try again. Might have been a temp. glitch

Extended the fret and pitch ranges of the three Mountain Dulcimer instruments
to conform with what are now the common ranges for modern chromatic dulcimers.

Dec-8: amending commit with no code change in order to trigger re-run of the
mtests given the merge of PR #7032.
@JeffRocchio
Copy link
Author

Success at last! Whew! Well, I figured this first, tentative, attempt at a pull request would be a learning experience; and I wasn't wrong about that. Thanks for helping me get to this point Jojo.

@Jojo-Schmitz
Copy link
Contributor

We've all once been through those initial issues ;-)

@vpereverzev vpereverzev merged commit 0a5e9e6 into musescore:3.x Dec 19, 2020
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.

4 participants