Skip to content

Conversation

@unwiredben
Copy link

This fixes an issue I had using the tool with filename completion by allowing the user to provide the .mid extension when specifying the base filename. It will strip that off before processing.

BTW, I'd appreciate it if you could label this PR as "hacktoberfest-accepted" even if you don't want to merge it so it will count towards my goal for the month.

@MLXXXp
Copy link
Owner

MLXXXp commented Oct 17, 2020

midi2tones is a fork of miditones. For things other than changes that @LenShustek has chosen not to incorporate into miditones, I try to keep them in sync (although I'm a bit behind on that).

For this reason, I suggest that you submit an equivalent PR for miditones. If it's accepted there, I'll port it into midi2tones or merge a submitted PR that does so. If it's not accepted, I'll still consider applying this PR, as I think it's a useful enhancement.

However,

  • I prefer that the commit message follow the "50/72" guidelines, as suggested by Tim Pope. The first line should be no more than 50 characters.

  • Consider also stripping off a .MID extension (in uppercase) to better accommodate case insensitive file systems.

  • The new version should be 1.1.0, since this is a backwards compatible enhancement, not a bug fix.

I've labelled this PR "hacktoberfest-accepted", as requested.

@unwiredben
Copy link
Author

Ah, I'd assumed that the original repo wasn't being maintained, since I only saw your changes, but now I see its had activity in 2019. I'll reformat my commit message and see about getting the PR accepted there. The idea for supporting ".MID" also is good, so I'll add that to my branch. Thanks!

This changes the command line handling so if the user
specifies the full name of the MIDI file, the code will silently
drop the extension and use that as the base name. This makes
the tool friendlier for people using filename completion, as they
don't have to remember to remove the ".mid" manually.
@unwiredben
Copy link
Author

Created PR for Len at LenShustek#23

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.

2 participants