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

Update Pull Request Template #1271

Merged
merged 12 commits into from
Apr 25, 2024
Merged

Update Pull Request Template #1271

merged 12 commits into from
Apr 25, 2024

Conversation

igordmn
Copy link
Collaborator

@igordmn igordmn commented Apr 15, 2024

  • Merge Proposed Changes and Issues fixed
  • Move it to the top
  • Describe Testing in more detail
  • Add Release Notes for the changelog automation.

The same template will be in the main repo and in Skiko (except the CLA section)

@igordmn igordmn marked this pull request as ready for review April 15, 2024 15:12
@igordmn igordmn requested a review from MatkovIvan April 15, 2024 15:13
@igordmn
Copy link
Collaborator Author

igordmn commented Apr 15, 2024

@MatkovIvan, could you please review? After we agree, I will ask others about this format

@igordmn igordmn requested a review from m-sasha April 16, 2024 08:21
@igordmn
Copy link
Collaborator Author

igordmn commented Apr 16, 2024

@m-sasha, could you also look at the template before I'll post a broader discussion with the team?

@igordmn igordmn requested a review from m-sasha April 16, 2024 11:17
-
-
-
[Optional] RelNote [Section/Subsection] Adds a feature
Copy link
Collaborator Author

@igordmn igordmn Apr 16, 2024

Choose a reason for hiding this comment

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

@m-sasha, in your new PR we have RelNote/Issues fixed as sections.

Will it be okay if they will be just lines as in this template? If not, let's discuss

Copy link

Choose a reason for hiding this comment

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

I don't mind either way.

Copy link
Collaborator Author

@igordmn igordmn Apr 16, 2024

Choose a reason for hiding this comment

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

Let's use single-line then.

It was chosen because it is compact, and it is nearby "Fixes" which will be used if no RelNote provided

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can have issues if we need to write large release notes. But let's start with the current option and change later if needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After looking at the final variant, it may be controversial 😅.

@m-sasha, @MatkovIvan, what do you prefer more?

1 2 3
image image image

Copy link

Choose a reason for hiding this comment

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

I wouldn't lead with the relnote, so I prefer 1 or 2

Copy link
Collaborator Author

@igordmn igordmn Apr 16, 2024

Choose a reason for hiding this comment

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

I prefer 2. And maybe we call it "Release Notes"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added as a section, fully reformatted

- Breaking changes
- Features
- Fixes
- Prerelease fixes
Copy link

Choose a reason for hiding this comment

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

What's the difference between prerelease fixes and regular fixes? Is the distinction important?

Copy link
Collaborator Author

@igordmn igordmn Apr 18, 2024

Choose a reason for hiding this comment

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

Prelease fixes are fixes of the bugs that we introduced in previous dev/beta release (as a regression, or in a new feature).

We'll include them in the next beta changelog, but won't include in the stable changelog.

I am not sure about adding a separate category for this. What do you think?

There is an option to keep only "Fixes", but add a separate indicator:

prerelease fix - add it if it fixes a bug introduced in a previous prerelease version (dev/beta). It will be excluded from the stable changelog
> Section - Subsection, prerelease fix
- Describe a change for adding it to https://github.com/JetBrains/compose-multiplatform/blob/master/CHANGELOG.md

Copy link
Collaborator Author

@igordmn igordmn Apr 18, 2024

Choose a reason for hiding this comment

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

Or:

prerelease fix - add it if it fixes a bug introduced in a previous prerelease version (dev/beta). It will be excluded from the stable changelog
> Section - Subsection
- _(prerelease fix)_ Describe a change for adding it to https://github.com/JetBrains/compose-multiplatform/blob/master/CHANGELOG.md

As we did in https://github.com/JetBrains/compose-multiplatform/releases/tag/v1.6.0-beta02

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I decided to keep it for now, but added a comment

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep - _(prerelease fix)_ mark for now - separate category takes too much space

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@igordmn igordmn requested a review from eymar April 23, 2024 19:44
> Prerelease fixes are fixes of bugs introduced in a previous prerelease version (dev/beta). It will be excluded from the stable changelog

Subsections:
- Multiple Platforms
Copy link
Collaborator

@eymar eymar Apr 24, 2024

Choose a reason for hiding this comment

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

So if a fix was for k/native and k/js,k/wasm. Do we choose "Multiple Platforms", despite the fix is not applicable for desktop?


Another note:
"Resources" and "Gradle Plugin" is a bit different category than platforms. They are rather "components".
Fixes in "Resources" and "Gradle plugin" can be platform specific as well.

But for simplicity we can keep it like you propose here. If it's not enough, we can add one more "category" later.

Copy link
Collaborator Author

@igordmn igordmn Apr 24, 2024

Choose a reason for hiding this comment

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

Do we choose "Multiple Platforms", despite the fix is not applicable for desktop?

Yes, it is to avoid adding an additional dimension - it can complicate things.

They are rather "components".
Fixes in "Resources" and "Gradle plugin" can be platform specific as well.

It is more about subsystems rather than targets and components.

We treat "Multiple Platforms", "Desktop", "Web", "iOS" as "Compose. Multiple Platforms", "Compose. Desktop", ... But we omit "Compose" as it is the main changelog for it.

@eymar
Copy link
Collaborator

eymar commented Apr 24, 2024

Failing web tests can be either ignored, or jb-main can be merged here (or rebased)

@igordmn igordmn changed the title Update PULL_REQUEST_TEMPLATE.md Update Pull Request Template Apr 24, 2024
@igordmn
Copy link
Collaborator Author

igordmn commented Apr 24, 2024

The main repo template: JetBrains/compose-multiplatform#4698

@igordmn igordmn merged commit f9b90c5 into jb-main Apr 25, 2024
4 of 6 checks passed
@igordmn igordmn deleted the igordmn-patch-4 branch April 25, 2024 14:01
igordmn added a commit to JetBrains/compose-multiplatform that referenced this pull request Apr 25, 2024
Introduce the same template used in [core
repo](JetBrains/compose-multiplatform-core#1271)
(except the CLA section)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants