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

[Bug] the action adds - in front of lines starting with @ #35

Closed
chicco785 opened this issue Jun 28, 2023 · 8 comments · Fixed by #38
Closed

[Bug] the action adds - in front of lines starting with @ #35

chicco785 opened this issue Jun 28, 2023 · 8 comments · Fixed by #38
Labels
bug Something isn't working

Comments

@chicco785
Copy link

in case of any previous release note including a line like:

- Add workflow to clean-up action cache on PR closure xxxxxxxxxxx (PR #8 by
  @chicco785)

the action changes the content to:

- Add workflow to clean-up action cache on PR closure xxxxxxxxxxx (PR #8 by
- @chicco785)
@stefanzweifel
Copy link
Owner

Thanks for reporting!

This sounds weird, but I will try to confirm this issue in the coming days. Might be, that the issue lies in an upstream dependency that converts the text to markdown.

@stefanzweifel stefanzweifel added the bug Something isn't working label Jun 28, 2023
@chicco785
Copy link
Author

Thanks for reporting!

This sounds weird, but I will try to confirm this issue in the coming days. Might be, that the issue lies in an upstream dependency that converts the text to markdown.

it took me a while to dig down to the source of the issue, hopefully i didn't it wrong :) no worries, at the moment I solved adding a step with a sed command after your action ;)

chicco785 added a commit to zaphiro-technologies/github-workflows that referenced this issue Jun 28, 2023
…weifel/changelog-updater-action@v1` (#20)

<!-- Thank you for submitting a pull request to our repository. -->

## Description

After tracking the different steps it turns out that the action
`stefanzweifel/changelog-updater-action@v1` in case of a previous
release note including a line like:

```
- Add workflow to clean-up action cache on PR closure xxxxxxxxxxx (PR #8 by
  @chicco785)
```

changes the content to:

```
- Add workflow to clean-up action cache on PR closure xxxxxxxxxxx (PR #8 by
- @chicco785)
```

so, we remove added `-` after the action execution

The bug is reported in the original repository
stefanzweifel/changelog-updater-action#35, and
the workaround can be removed in the future if the issue is solved.

## Changes Made

include a step using a sed script to remove the added `-`.

## Related Issues

Fixes #21

## Checklist

<!-- Please check off the following items by putting an "x" in the box:
-->

- [x] I have used a PR title that is descriptive enough for a release
note.
- [x] I have tested these changes locally.
- [x] I have added appropriate tests or updated existing tests.
- [ ] I have tested these changes on a dedicated VM or a customer VM
[name of the VM]
- [ ] I have added appropriate documentation or updated existing
documentation.

---------

Co-authored-by: Bot <[email protected]>
@stefanzweifel
Copy link
Owner

I was able to reproduce this issue. The error is not happening in this project, but in a dependency down the line which I also maintain. 😎

I've opened a PR with the failing test here: stefanzweifel/commonmark-markdown-renderer#7
The code in question, that breaks this is ListItemRenderer.php and ListBlockRenderer.php.


All markdown rendering apps that I use (Obsidian, iA Writer) convert the example you provided to a single line item without a line break in it.

So this …

- Add workflow to clean-up action cache on PR closure xxxxxxxxxxx (PR #8 by
  @chicco785)

turns into this.

<li>Add workflow to clean-up action cache on PR closure xxxxxxxxxxx (PR #8 by @chicco785)</li>

I would adopt this logic and update the package, to replace all line breaks (\n) in a list item with a space ( ).
Otherwise I would have to rewrite the ListBlockRenderer.php to distinguish line items differently. (And I have no idea how I could do that right now.)

Or do you expect the final document to keep the original formatting?
To be this:

- Add workflow to clean-up action cache on PR closure xxxxxxxxxxx (PR #8 by
  @chicco785)

instead of this:

- Add workflow to clean-up action cache on PR closure xxxxxxxxxxx (PR #8 by @chicco785)

@chicco785
Copy link
Author

I think the problem may be related to the combination of your code with prettier.io. I am currently on holidays, so I can’t provide more details right now, but will be happy to do so in 2 weeks.

@stefanzweifel
Copy link
Owner

Sure, no problem. Enjoy your vacation.

@chicco785
Copy link
Author

hi @stefanzweifel
i am back :)

basically, we run two workflows:

  1. generates release release notes (https://github.com/zaphiro-technologies/github-workflows/blob/main/.github/workflows/release-notes.yaml)
  2. check and pretty markdown (https://github.com/zaphiro-technologies/github-workflows/blob/main/.github/workflows/markdown.yaml)

so when release notes are updated, the workflows are triggered again, and the prettier changes the formatting of the release notes. the combination of the two actions may cause the weird behaviour. since, the prettier act on the release note and breaks it into multiple lines, and then the release note action process it again adding to the start of the new line the -, despite this is still actually part of the existing bullet. (I hope it's clear)

@stefanzweifel
Copy link
Owner

Thanks for the feedback. So the wrapping is coming from prettier.
I think I will simply fix and merge my PR here and update all upstream projects accordingly.

@stefanzweifel
Copy link
Owner

@chicco785 This issue should now be fixed.

As always, this was a bit more complex than expected. The Markdown-to-Markdown converter couldn't handle multi-line list items yet. This has now been fixed in stefanzweifel/commonmark-markdown-renderer#7.

The underlying CLI of this Action has been updated as well (stefanzweifel/php-changelog-updater#44).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants