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

Handle trailing newline in macro definitions #361

Merged
merged 2 commits into from
Apr 4, 2024
Merged

Handle trailing newline in macro definitions #361

merged 2 commits into from
Apr 4, 2024

Conversation

nikitych
Copy link
Contributor

@nikitych nikitych commented Apr 3, 2024

Fixes #360.

Copy link
Contributor

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/51088f7327484e5eba82fd07213c3e14

✔️ pre-commit SUCCESS in 2m 18s
✔️ specfile-tests-rpm-deps SUCCESS in 1m 11s
✔️ specfile-tests-pip-deps SUCCESS in 1m 04s

Copy link
Member

@nforro nforro left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

However, I believe this is also a parsing issue. The ending newline character should be part of the body, not included in _whitespace. To make that right, the regex here should be replaced with ([^\S\n]+)$ (not to match \n characters):

tokens = re.split(r"(\s+)$", body, maxsplit=1)

If you are fine with it, I will merge the PR as it is and fix the parsing myself afterwards.

@nikitych
Copy link
Contributor Author

nikitych commented Apr 3, 2024

I'll rewrite PR to add \n to the body.

There are also a couple of technically errors of missing \n here and here. I spent some time but could not figure out how to exploit the non-excluded empty string. So I could fix them as well, but without adding tests.

@nforro
Copy link
Member

nforro commented Apr 3, 2024

I'll rewrite PR to add \n to the body.

Thanks! Would you also add that case to test_parse?

There are also a couple of technically errors of missing \n here and here. I spent some time but could not figure out how to exploit the non-excluded empty string. So I could fix them as well, but without adding tests.

Good point, but I don't think it matters here, empty lines don't have to be excluded because they can't possibly contain a start of a section. But feel free to replace splitlines() with split("\n") (is that the fix you had in mind?).

Copy link
Contributor

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/0a7d01d5fbfd4657b9fc137779544627

✔️ pre-commit SUCCESS in 2m 12s
✔️ specfile-tests-rpm-deps SUCCESS in 1m 11s
✔️ specfile-tests-pip-deps SUCCESS in 1m 09s

Copy link
Member

@nforro nforro left a comment

Choose a reason for hiding this comment

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

Thanks!

@nforro nforro added the mergeit Zuul, merge it! label Apr 4, 2024
Copy link
Contributor

Build succeeded (gate pipeline).
https://softwarefactory-project.io/zuul/t/packit-service/buildset/d0cdcd11874b479c99ce699a72857e9a

✔️ pre-commit SUCCESS in 2m 49s

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 6fc5bee into packit:main Apr 4, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mergeit Zuul, merge it!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MacroDefinition ignore body trailing newline
2 participants