Skip to content

Conversation

FelippeRoza
Copy link
Collaborator

@FelippeRoza FelippeRoza commented May 15, 2017

Feature implementation in order to solve #30.

@uliska
Copy link
Collaborator

uliska commented May 17, 2017

Thank you for this pull request!

It is a good idea to start with something like this to get familiar with the code and maybe also with the collaboration traditions etc.

As per your GSoC proposal you will add a number of such features to increase the MusicXML export's coverage. This is good and we will benefit greatly from this. But be aware that the GSoC project is not meant to exploit you to work on repetitive tasks that others won't do. I suggest that you continue adding features like this until you are really comfortable with the coding etc. and at some point move to something more challenging. (You may at any time get back to this and add something here and there if you have a time window that is too small for more demanding tasks).

I'm not familiar with the exporter code, so I can't review the PR in detail (please have a look, @PeterBjuhr). All I can say is that your code looks generally convincing, but I don't know how well it fits into the existing structure and coding style.

I did pull your branch and tested it, and I can say that it nearly works. Basically the result is correct, but it should read new-system="yes" instead of new_system="yes". Exported files look nearly correct, and after manually editing this single problem in the XML file MuseScore imports the file correctly, respecting the line breaks.

(PS: LilyPond's musicxml2ly does not recognize line breaks when importing, which is known and has tracker item #2321.) You may want to look into this as well (as musicxml2ly is also written in Python), but that's completely optional.
PPS: I add one line comment to the commits to tell you something about our coding conventions.)

def parse_text(self, ly_text, filename=None):
"""Parse the LilyPond source specified as text.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file includes a number of whitespace-only changes in the commit, which you should try to avoid. As I describe in frescobaldi/frescobaldi#924 we have the problem that there are many whitespace-only lines in our codebase. It seems you use an editor that automatically removes this upon saving, and while this is generally a good thing (Frescobaldi offers this option too) it triggers "noise" in the commit.

Please configure your editor not to automatically remove such lines.
(This is not meant as criticism. It's a regular issue).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was something with my editor indeed and I already fixed it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you. Actually you have to "break" it to make it consistent with our state of the code base ;-) I would really prefer all the code to be without these false-empty lines.

@FelippeRoza
Copy link
Collaborator Author

Hi Urs!

I suggest that you continue adding features like this until you are really comfortable with the coding etc. and at some point move to something more challenging.

That's my plan. I'm just getting familiar with the code and implementing such features is a nice way to do so. I confess I don't have a clear idea about how to improve the MusicXML export yet. I guess it would be great if I could find examples (ly files) where something wrong happens.

Basically the result is correct, but it should read new-system="yes" instead of new_system="yes".

It was a typo and I'll fix it.

@uliska
Copy link
Collaborator

uliska commented May 17, 2017

Two more tiny comments.

When you insert #30 anywhere in a Github comment (and probably commit messages too) Github will automatically create a link. See the difference between issue#30 and issue #30? As an additional benefit the mention will be listed on the target's page.

When you have a commit that is related to a tracker item you can use something like affects #30 in the commit message, then the commit will also show up in the issue's discussion thread.

Finally, you can insert closes #30 in a commit message. In this case the tracker item will be closed as soon as the commit is merged into master.

@PeterBjuhr
Copy link
Collaborator

Thanks for the pull request @FelippeRoza and for the review @uliska ! I'm just starting to get back to this but I noticed that there already is a similar PR waiting #89!

@PeterBjuhr PeterBjuhr merged commit e4b3258 into frescobaldi:master Sep 4, 2017
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.

3 participants