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

Fix handling of multiple nodes within list item #166

Merged
merged 2 commits into from
Jun 1, 2023

Conversation

JojOatXGME
Copy link
Contributor

@JojOatXGME JojOatXGME commented Apr 22, 2023

Pull Request Details

This pull request is supposed to fix #133 and fix #147. At least it works in my case. I am not really familiar with Kotlin or the implementation details of this plugin. I basically just searched for usages of last() within list items.

Description

This PR replaces the usage of last() to avoid discarding previous nodes.

Related Issue

Motivation and Context

See the issues above. I personally run into this problem while trying to update the changelog plugin on the nix-idea project. The plugin specifically breaks for the following part of the changelog.

https://github.com/NixOS/nix-idea/blob/59cc3e911fb468b225a92c1e331d135ec56e01cf/CHANGELOG.md?plain=1#L78-L90

How Has This Been Tested

I tested the changes on the nix-idea project, where I run into the issues above. I locally published a snapshot and used it in the project. I was not familiar enough with Kotlin and the project setup of this repository to make more elaborate tests.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the README.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@YannCebron
Copy link
Member

Thanks for your contribution. Please rebase for GH actions to run successfully. Could we have some tests for this? 👍

@JojOatXGME
Copy link
Contributor Author

JojOatXGME commented Apr 24, 2023

I added a test for the patchChangelog task (#147). While I also tested getChangelog (#133) locally to be sure, I did not commit a test for that task. Both tasks are using the same implementation, and I didn't want to write essentially the same test twice.

If you look at my changes in Changelog.kt, you may notice that the code I changed is duplicated for unassignedItems and sectionsWithItems. I think my test is currently only covering the usage of sectionsWithItems. I am not sure about the background of unassignedItems.

@YannCebron
Copy link
Member

please rebase your branch on main

@JojOatXGME
Copy link
Contributor Author

@YannCebron, I looked into the build failures caused by dorny/test-reporter myself for a moment and there seem to be four open issues regarding this error:

I noticed that you referenced the last one in your previous attempt to fix the issue. The other tickets also mention the statuses: write permission. So maybe you have to add it as well. However, I think it is quite likely that the following is actually preventing your changes from being effective (GitHub Actions documentation):

You can use the permissions key to add and remove read permissions for forked repositories, but typically you can't grant write access.

I am also not sure if providing write access to pull requests is a great idea. It would allow other people to mess with checks and statuses on your repository without being a member. So, I am currently not sure if there is a good solution for using this action in pull requests.

@hsz hsz merged commit f15931d into JetBrains:main Jun 1, 2023
@hsz
Copy link
Member

hsz commented Jun 1, 2023

Good job, Johannes — thank you!

@hsz hsz added this to the next milestone Jun 1, 2023
@JojOatXGME JojOatXGME deleted the fix_multiple_nodes_in_list branch June 1, 2023 21:31
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.

Sub-bullets remove root bullet getChangelog task loses pre-formatted text
3 participants