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

Refine PULL_REQUEST_TEMPLATE.md #12749

Merged
merged 13 commits into from
Mar 16, 2025
Merged

Refine PULL_REQUEST_TEMPLATE.md #12749

merged 13 commits into from
Mar 16, 2025

Conversation

koppor
Copy link
Member

@koppor koppor commented Mar 16, 2025

Refinement of PULL_REQUEST_TEMPLATE triggered by #12662 (comment) / #12741

  • I own the copyright of the code submitted and I licence it under the MIT license
  • [/] Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@koppor koppor requested a review from subhramit March 16, 2025 17:45
subhramit

This comment was marked as outdated.

@@ -1,21 +1,27 @@
<!-- YOU HAVE TO MODIFY THIS TEXT TO FIT YOUR PR. OTHERWISE, YOUR PR WILL BE CLOSED WITHOUT FURTHER COMMENT. -->

Describe the changes you have made here: what, why, ...
Describe the changes you have made here: what, where, why, ...
If it is WIP, please open a draft pull request. In that case, outline your intended next steps. Do you need feedback? Will you continue in parallel? ...
Copy link
Member

Choose a reason for hiding this comment

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

99% of PRs are work in progress. The major difference between Draft and real PR should in theory be something that is deemed mergeable or not, but in practice this fine line is fluid and always will be, because one doesn't know what one doesn't know and a review might bring to light things that was not considered beforehand and that's ok. Draft means, the one that opens the PR has the intention or the knowledge that the PR is not finished yet and will continue to work on it.

The only thing that is required for maintainers is to know when to start a review.

Copy link
Member

Choose a reason for hiding this comment

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

If it is WIP "If you are not yet sure about your approach -" does this convey it better?

Copy link
Member

Choose a reason for hiding this comment

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

Ah no, same loophole

Copy link
Member

Choose a reason for hiding this comment

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

This should be better now.


<!-- LINK THE ISSUE WITH THE "Closes" KEYWORD -->
<!-- Example: Closes (link) OR Closes #xyz -->

### Mandatory checks

<!--
- Go through the list below. Please don't remove any items.
- [x] done; [ ] not done / not applicable
Go throgh the checklist below. It is mandatory, even if you submit a draft pull request.
Copy link
Member

Choose a reason for hiding this comment

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

Try to avoid conditional statements. It makes sentences complicated to read.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Go throgh the checklist below. It is mandatory, even if you submit a draft pull request.
Go throgh the checklist below. It is mandatory.

Copy link
Member

@subhramit subhramit Mar 16, 2025

Choose a reason for hiding this comment

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

It is more of an "unconditional" statement. Would this be better:

Suggested change
Go throgh the checklist below. It is mandatory, even if you submit a draft pull request.
Go throgh the checklist below. It is mandatory, even for a draft pull request.

- Go through the list below. Please don't remove any items.
- [x] done; [ ] not done / not applicable
Go throgh the checklist below. It is mandatory, even if you submit a draft pull request.
For instance, a screenshot helps in reviewing - even if your code is not 100% complete.
Copy link
Member

Choose a reason for hiding this comment

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

I still don't like this line. It's too much text that doubles down on a single topic.

Suggested change
For instance, a screenshot helps in reviewing - even if your code is not 100% complete.

Copy link
Member

@subhramit subhramit Mar 16, 2025

Choose a reason for hiding this comment

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

Trigger for this was here, so I am guessing idea was to put it somewhere. Keeping this line OR keeping #12749 (comment) would suffice, so I'll apply your suggestion on this one (removal).

Copy link
Member

@subhramit subhramit left a comment

Choose a reason for hiding this comment

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

Seems good to go

Copy link
Contributor

github-actions bot commented Mar 16, 2025

The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build.

Keep ALL the items. Mark them as follows:
[x] done
[ ] not done
[/] not applicable
Copy link
Member Author

Choose a reason for hiding this comment

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

[/] is not supported by GitHub:

image

But I find it helpful to separate n/a from not done ^^

@koppor koppor added this pull request to the merge queue Mar 16, 2025
@Siedlerchr
Copy link
Member

don't make it too complicated. Students's won't read it anyways ;)

Merged via the queue into main with commit 55bc0cf Mar 16, 2025
32 checks passed
@koppor koppor deleted the koppor-patch-1 branch March 16, 2025 20:08
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.

4 participants