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

.show('flat'), support for http://flat.io #846

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Meekohi
Copy link
Contributor

@Meekohi Meekohi commented Feb 10, 2021

Adds ConverterFlatio which uploads XML to flat.io on .write('flat') and opens a browser tab to the file on .show('flat').

  • Introduces a environLocal['flatioAuthToken'] which is required to use their API. There is a helpful error message if you forget to set it and try to do .show('flat)'
  • This does add a dependency on requests and webbrowser which is slightly weird. Might be better to leave them out and just instruct people to pip install them if needed in the documentation?

@Meekohi
Copy link
Contributor Author

Meekohi commented Feb 10, 2021

Looks like I need to figure out how to get import 'requests' to pass pylint, but first I figure I would check if this a feature anyone except me actually wants ;)

@mscuthbert
Copy link
Member

can we make the format 'flatio' -- it's just that 'flat' has such a prominent other meaning in m21. OR since it's actually a subformat of musicxml, I think that musicxml.flat should also work. I'll just need to think a little bit about having a format that I can't fully test and whether I'm willing to commit to the support costs of maintaining this for the next decade. I think so, but it's something I'll have to think about. Supporting MuseScore problems and Lilypond problems, etc. has been a lot, and I know I'll get a lot of emails about "how do I set up my flatIOAuthToken" which I don't know the answer to. But it is something really good I'd think.

@Meekohi
Copy link
Contributor Author

Meekohi commented Feb 10, 2021

Yeah I definitely appreciate the extra maintenance burden -- let me know what you decide. On the upside, there isn't much technical trickery to worry about since it's just "send raw XML" and then let flat.io deal with it.

@mscuthbert
Copy link
Member

I think what people will ask for help with is "What do I name my app?" and "What are the authroization scopes I need?" since they don't think of themselves as using an APP and, "How do I put that in environment?" since the vast majority of users never configure their environment. So if the docs for that can be a more step-by-step description, or even a prompt to paste the token into the script to update their environment (see the configure package for instance) then it'll be less of a hassle in the future.

I'm leaning "yes" on this, but I think we'll tag it as an "Experimental feature that may be removed in future versions"

setup.py Outdated
Comment on lines 39 to 40
"requests",
"webbrowser"
Copy link
Member

Choose a reason for hiding this comment

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

Hi @Meekohi. When it's time to retun to this, could you also update requirements.txt? I think this section of setup.py is just for requirements-minimum, i.e. not packages that we import late and want to keep optional. I suppose Michael needs to decide whether requests will be mandatory. Historically music21 has had users brand new to Python who only install music21.

Copy link
Member

Choose a reason for hiding this comment

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

also, webbrowser is in the standard lib, so we don't need to call it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Roger that -- I do plan on revisiting this PR, but we've gotten dragged onto other work for now.

'''
Uploads MusicXML to flat.io via their API.
'''
import requests
Copy link
Member

Choose a reason for hiding this comment

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

requests isn't in the requirements.txt. Is it possible to do this with the python standard library? If not, it needs to be added to requirements.txt

Copy link
Member

Choose a reason for hiding this comment

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

Oops, was already mentioned above. I think that given that the Python std library docs already suggest using requests instead, and it's only not in the std library because it gets security fixes faster than other libraries, it's probably okay. So we just need the format name changed and a mechanism for helping users get their API codes, etc.

@mscuthbert
Copy link
Member

Hello -- just as a note (from the music21list Google Group) I'm taking a sabbatical from reviewing music21 issues and PRs until at least Jan 1, 2024, so this PR will be deferred until then.

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