convert-ly rule for tocItem

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

convert-ly rule for tocItem

Timothy Lanfear
Applying convert-ly (2.21.5) to this file produces illegal output with
the first double quote escaped by a backslash

\version "2.20.0"

\tocItem "Score A"
score { { a'1 } }

Output

\version "2.21.2"

\tocItem \markup \"Score A"
\score { { a'1 } }

--
Timothy Lanfear, Bristol, UK.


_______________________________________________
bug-lilypond mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/bug-lilypond
Timothy Lanfear, Bristol, UK.
Reply | Threaded
Open this post in threaded view
|

Re: convert-ly rule for tocItem

Noeck
I can confirm the issue.

And I think it is a bug in python/convertrules.py:4267. I found this:

    s = re.sub(r'\\tocItem\s+\"', r'\\tocItem \\markup \"', s)

In the replacement string the " does not need to be escaped. And with
the raw string r'' the backslash is interpreted literally.



Am 22.08.20 um 20:33 schrieb Timothy Lanfear:
> \version "2.20.0"
>
> \tocItem "Score A"
>  score { { a'1 } }
  ^
Btw, a backslash is missing here.


_______________________________________________
bug-lilypond mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/bug-lilypond
Reply | Threaded
Open this post in threaded view
|

Re: convert-ly rule for tocItem

Noeck
Btw, pylint reveals (among many other things) 27 warnings about:

W1401: Anomalous backslash in string: '\m'. String constant might be
missing an r prefix. (anomalous-backslash-in-string)

which shows that backslashes are not placed properly. In the case of
`\m` this is interpreted as `\\m` but in cases of `\n` it is a new line.


As discussed elsewhere, it is very far from PEP8 standards.

+-----------+-------+
|type       |number |
+===========+=======+
|convention |1176   |
+-----------+-------+
|refactor   |9      |
+-----------+-------+
|warning    |45     |
+-----------+-------+
|error      |465    |
+-----------+-------+


_______________________________________________
bug-lilypond mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/bug-lilypond
Reply | Threaded
Open this post in threaded view
|

Re: convert-ly rule for tocItem

Bugs mailing list
In reply to this post by Noeck
Am Sonntag, den 23.08.2020, 07:51 +0200 schrieb Noeck:
> I can confirm the issue.

Yep...

> And I think it is a bug in python/convertrules.py:4267. I found this:
>
>     s = re.sub(r'\\tocItem\s+\"', r'\\tocItem \\markup \"', s)
>
> In the replacement string the " does not need to be escaped. And with
> the raw string r'' the backslash is interpreted literally.

... and yep. Not sure it needs to be escaped in the regular expression?
I've opened https://gitlab.com/lilypond/lilypond/-/issues/6024 just so
it doesn't get lost. If you want to, feel free to submit the fix as a
merge request 😉


> Am 22.08.20 um 20:33 schrieb Timothy Lanfear:
> > \version "2.20.0"
> >
> > \tocItem "Score A"
> >  score { { a'1 } }
>
>   ^
> Btw, a backslash is missing here.
>
>
> _______________________________________________
> bug-lilypond mailing list
> [hidden email]
>
> https://lists.gnu.org/mailman/listinfo/bug-lilypond
>
>

_______________________________________________
bug-lilypond mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/bug-lilypond

signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: convert-ly rule for tocItem

Jean ABOU SAMRA
In reply to this post by Timothy Lanfear
I can confirm the issue. And I think it is a bug in python/convertrules.py:4267. I found this:
s = re.sub(r'\\tocItem\s+\"', r'\\tocItem \\markup \"', s) In the replacement string the " does not need to be escaped. And with the raw string r'' the backslash is interpreted literally.

JAS-> Correct.

Btw, pylint reveals (among many other things) 27 warnings about: W1401: Anomalous backslash in string: '\m'. String constant might be missing an r prefix. (anomalous-backslash-in-string)
which shows that backslashes are not placed properly. In the case of `\m` this is interpreted as `\\m` but in cases of `\n` it is a new line.

JAS-> I've scratched that itch nearly everywhere in the other Python files, but for convert-ly I was just too tired and it made little sense until we actually had a linter in CI because convertrules.py is read infrequently.

... and yep. Not sure it needs to be escaped in the regular expression?

JAS-> I don't think it needs this escape, though I can't check.

I've opened
https://gitlab.com/lilypond/lilypond/-/issues/6024
just so it doesn't get lost. If you want to, feel free to submit the fix as a merge request 😉

 JAS-> Absolutely!

Jean
_______________________________________________
bug-lilypond mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/bug-lilypond
Reply | Threaded
Open this post in threaded view
|

Re: convert-ly rule for tocItem

Noeck
In reply to this post by Bugs mailing list


Am 24.08.20 um 19:54 schrieb Jonas Hahnfeld:
>  feel free to submit the fix as a merge request 😉

Hi Jonas,

I would like to do so and I have the local changes. I am joram-b on gitlab.

Could you give me a start?
Should I fork lilypond or can I push a branch to lilypond for review?
Is there a naming scheme like dev/joram-b/... or should the issue 6024
be referenced?
Should I branch from current master?

Best,
Joram

_______________________________________________
bug-lilypond mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/bug-lilypond
Reply | Threaded
Open this post in threaded view
|

Re: convert-ly rule for tocItem

Bugs mailing list
Am Sonntag, den 30.08.2020, 16:48 +0200 schrieb Noeck:
> Am 24.08.20 um 19:54 schrieb Jonas Hahnfeld:
> >  feel free to submit the fix as a merge request 😉
>
> Hi Jonas,
>
> I would like to do so and I have the local changes. I am joram-b on gitlab.

Great! :-)

> Could you give me a start?

The CG has some basic information here:
https://lilypond.org/doc/v2.21/Documentation/contributor/uploading-a-patch-for-review

and the underlying process is described here:
https://lilypond.org/doc/v2.21/Documentation/contributor/the-patch-review-cycle
(that's mostly FYI, we'll let you know if we expect you to do something
other than the first link)

> Should I fork lilypond or can I push a branch to lilypond for review?

Yes, a fork would be easiest here. Just open a Merge Request from
there.

> Is there a naming scheme like dev/joram-b/... or should the issue 6024
> be referenced?

The naming scheme only applies to branches in the main repository,
you're free to use whatever you like in your fork (except you shouldn't
just push to master, that results in all kinds of problems). It would
be good to put "Closes #6024" in the last line of the commit message.

> Should I branch from current master?

That would be perfect, but a slightly older version of master as the
base is also fine as long as it doesn't contain other unsubmitted
changes.

Hope this helps, if not please don't hesitate to ask!
Jonas

> Best,
> Joram

_______________________________________________
bug-lilypond mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/bug-lilypond

signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: convert-ly rule for tocItem

Noeck
Hi Jonas,

thank you for your help and thank you for driving the switch to gitlab.
I tried several times in the last 10 years to contribute to LilyPond but
it was always complicated and I never succeeded. I'll see how this works
out now, but it is really straight forward so far. Thank you!

The merge request is on gitlab:
https://gitlab.com/lilypond/lilypond/-/merge_requests/363
I put the changes in 5 separate commits.

Is it correct to create a merge request to master? It is only a direct
push to master that is problematic, right?

Cheers,
Joram


_______________________________________________
bug-lilypond mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/bug-lilypond
Reply | Threaded
Open this post in threaded view
|

Re: convert-ly rule for tocItem

Bugs mailing list
Am Sonntag, den 30.08.2020, 18:47 +0200 schrieb Noeck:

> Hi Jonas,
>
> thank you for your help and thank you for driving the switch to gitlab.
> I tried several times in the last 10 years to contribute to LilyPond but
> it was always complicated and I never succeeded. I'll see how this works
> out now, but it is really straight forward so far. Thank you!
>
> The merge request is on gitlab:
> https://gitlab.com/lilypond/lilypond/-/merge_requests/363
> I put the changes in 5 separate commits.
Good, please see my review comment.

> Is it correct to create a merge request to master? It is only a direct
> push to master that is problematic, right?

Yes, targeting master is correct. Changes can only be merged to master
after testing, which is enforced by GitLab.

Jonas

_______________________________________________
bug-lilypond mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/bug-lilypond

signature.asc (499 bytes) Download Attachment