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

Update miditime import #568

Merged
merged 2 commits into from
Aug 29, 2017
Merged

Update miditime import #568

merged 2 commits into from
Aug 29, 2017

Conversation

rlskoeser
Copy link
Contributor

The miditime import in the example didn't work for me, I had to switch it to this version. I used pip install and got miditime==1.1.3. (The project documentation shows the old import, but that doesn't work.)

I thought it might be worth updating, because if people aren't familiar with Python they may not be able to figure it out. You might want to add something about the version of miditime or document the older syntax also, but I don't know where that should go.

NOTICE: Are you attempting to submit a lesson to the Programming Historian? We no longer accept lesson submissions by pull request to this repository. Please consult our guidelines for submitting a lesson for further instructions.

If your pull request instead concerns some issue with the formatting or functioning of our site, or some error in an existing lesson, please see our technical contribution guidelines: https://github.com/programminghistorian/jekyll/wiki/Making-technical-contributions

The miditime import in the example didn't work for me, I had to switch it to this version.  I used `pip install` and got `miditime==1.1.3`.    (The project documentation shows the old import, but that doesn't work.)

I thought it might be worth updating, because if people aren't familiar with Python they may not be able to figure it out.   You might want to add something about the version of miditime or document the older syntax also, but I don't know where that should go.
@ianmilligan1
Copy link
Contributor

Thanks @rlskoeser!

Pinging @shawngraham here – thoughts Shawn?

@mdlincoln
Copy link
Contributor

(fyi the build break is due to #569, and not due to anything in this PR)

@shawngraham
Copy link

just saw this. Thank you @rlskoeser . I'm rather swamped at the moment, but I guess I'd better add the info re versions and so on. Appreciate the headsup.

@ianmilligan1
Copy link
Contributor

Do you have any thoughts on merging this, @shawngraham?

@shawngraham
Copy link

@ianmilligan1 i'm not sure what you want to merge? Life has intervened, and I won't be able to give this any serious attention for the next few weeks unfortunately

@ianmilligan1
Copy link
Contributor

There's just one line of changed code in the pull request. See screenshot or click on "Files Changed" up above. Is this OK to merge in?
screen shot 2017-08-29 at 10 45 17 am

@rlskoeser
Copy link
Contributor Author

@ianmilligan1 it's possible that change won't work in older versions of miditime, I'm not sure when it changed. if it would help, I could update with a comment to that effect with a note that people should try the older import if the new one doesn't work?

@ianmilligan1
Copy link
Contributor

Sure, why don't you do that, and then we can see if @shawngraham is happy with the note?

@rlskoeser
Copy link
Contributor Author

rlskoeser commented Aug 29, 2017

@ianmilligan1 @shawngraham I added a brief comment in the python sample code with a note about the old syntax. (Sorry I didn't just include that the first time.)

@ianmilligan1
Copy link
Contributor

Great, thanks so much @rlskoeser! Let me know @shawngraham, happy to merge if you are fine with this minor tweak.

@shawngraham
Copy link

I'm ok with it. Thanks @rlskoeser ! much appreciated

@ianmilligan1 ianmilligan1 merged commit f9045a3 into programminghistorian:gh-pages Aug 29, 2017
@ianmilligan1
Copy link
Contributor

Great, thanks all.

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

Successfully merging this pull request may close these issues.

4 participants