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

chore: update pull request template #5221

Merged
merged 9 commits into from
Nov 5, 2024
Merged

chore: update pull request template #5221

merged 9 commits into from
Nov 5, 2024

Conversation

Akuukis
Copy link
Contributor

@Akuukis Akuukis commented Nov 4, 2024

💭 Notes

See the new Pull Request process here.

@Akuukis Akuukis changed the title chore: update PR template chore: update pull request template Nov 4, 2024
@Akuukis
Copy link
Contributor Author

Akuukis commented Nov 4, 2024

Preview of the template:


🗒️ Checklist

  1. run linter locally
  2. update all related docs (API, README, inline, etc.), if any
  3. draft PR with a title <type>(<scope>)<!>: <title> TASK-1234
  4. tag PR: at least frontend or backend unless it's global
  5. fill in the template below and delete template comments
  6. review thyself: read the diff and repro the preview as written
  7. open PR & confirm that CI passes
  8. request reviewers, if needed
  9. delete this section before merging

📣 Summary

📖 Description

👷 Description for instance maintainers

👀 Preview steps

Bug template:

  1. ℹ️ have an account and a project
  2. do this
  3. do that
  4. 🔴 [on main] notice that this isn't anywhere BUT it should be here
  5. 🟢 [on PR] notice that this is here

Feature/no-change template:

  1. ℹ️ have account and a project
  2. do this
  3. do that
  4. 🟢 notice that this is there
  5. do that another thing
  6. 🟢 notice that this changed like that

💭 Notes

@JacquelineMorrissette
Copy link
Contributor

My only concern about the template is the emoji in the summary. This shouldn't be a blocker, but updating the python scripts to recognize the emoji might be a bit tricky

@noliveleger
Copy link
Contributor

noliveleger commented Nov 4, 2024

My only concern about the template is the emoji in the summary. This shouldn't be a blocker, but updating the python scripts to recognize the emoji might be a bit tricky

@JacquelineMorrissette , it should not be a problem.

Type "help", "copyright", "credits" or "license" for more information.
>>> s = '📖 '
>>> s == '📖 '
True

@JacquelineMorrissette
Copy link
Contributor

My only concern about the template is the emoji in the summary. This shouldn't be a blocker, but updating the python scripts to recognize the emoji might be a bit tricky

@JacquelineMorrissette , it should not be a problem.

Type "help", "copyright", "credits" or "license" for more information.
>>> s = '📖 '
>>> s == '📖 '
True

I picked on #5113 just to try it but I can't seem to get it to work with the release notes script:

        if reading_state is None:
            standardized_line = condense_spaces(line).lower()
            if standardized_line == '## 📣Description':
                reading_state = 'description'
            elif standardized_line == '## related issues':
                reading_state = 'related issues'
            continue

What I was expecting:
[kpi#5113](https://github.com/kobotoolbox/kpi/pull/5113) Group submission access logs<br>Updates the access logs endpoints to return submissions grouped together by hour to avoid floods of individual submission logs. Also fixes a bug where submissions to the v1 endpoint were not being counted as submissions.

What I got:
[kpi#5113](https://github.com/kobotoolbox/kpi/pull/5113),Group submission access logs

@JacquelineMorrissette
Copy link
Contributor

Like I said, it shouldn't be a blocker, but I'm probably going to need some help to get the scripts working again

@Akuukis
Copy link
Contributor Author

Akuukis commented Nov 4, 2024

@JacquelineMorrissette I see a typo in your script ('## 📣Description' -> ### 📖 Description), maybe that's all it takes. Anyways, it's fixable and not blocking as noliveleger showed python works with emojis in principle.

@JacquelineMorrissette
Copy link
Contributor

It's not a typo, I picked an emoji just to test. It matches the emoji I added to the description heading in #5113

@Akuukis Akuukis self-assigned this Nov 4, 2024
@rgraber
Copy link
Contributor

rgraber commented Nov 4, 2024

Nit: can we not delete the checklist after? I find it useful to keep it around

@Akuukis
Copy link
Contributor Author

Akuukis commented Nov 4, 2024

@rgraber good point, to allow to check-uncheck-check things as one works forth-n-back with reviewers. Loosened the last checklist item to "delete this section before merging"

@Akuukis Akuukis merged commit 697ebe5 into main Nov 5, 2024
7 checks passed
@Akuukis Akuukis deleted the pr-template branch November 5, 2024 08:46
@p2edwards p2edwards added the workflow Related to development process label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
workflow Related to development process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants