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

[Decision] Commit function #4777

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

flo91
Copy link
Collaborator

@flo91 flo91 commented Dec 14, 2022

This PR is for working on the decision about the commit function (communicating the current phase to plugins).

It should at least reach the state "solutions clear" before closing this PR.

Basics

  • Short descriptions of your changes are in the release notes
    (added as entry in doc/news/_preparation_next_release.md which
    contains _(my name)_)
    Please always add something to the release notes.
  • Details of what you changed are in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, are in the commit messages.
  • The buildservers are happy. If not, fix in this order:
    • add a line in doc/news/_preparation_next_release.md
    • reformat the code with scripts/dev/reformat-all
    • make all unit tests pass
    • fix all memleaks
  • The PR is rebased with current master.

Checklist

  • I added unit tests for my code
  • I fully described what my PR does in the documentation
    (not in the PR description)
  • I fixed all affected documentation (see Documentation Guidelines)
  • I fixed all affected decisions (see Decision Process)
  • I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
  • I updated all meta data (e.g. README.md of plugins and METADATA.ini)
  • I mentioned every code not directly written by me in reuse syntax

Review

Labels

  • Add the "work in progress" label if you do not want the PR to be reviewed yet.
  • Add the "ready to merge" label if the basics are fulfilled and no further pushes are planned by you.

Copy link
Member

@kodebach kodebach left a comment

Choose a reason for hiding this comment

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

Please update the existing decision to match the new template. Specifically, please switch to using subsections in the "Considered Alternatives". That makes is a lot easier to read, write and talk about options.

IMO the decision can be moved to at least "In Discussion", but probably even "In Progress".

Copy link
Contributor

@eiskasten eiskasten left a comment

Choose a reason for hiding this comment

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

Well done! May you give some examples for function which potentially get called multiple times? I.e. if they are contract functions, plugin internal functions, etc.

@flo91 flo91 force-pushed the decision-commitFunction branch 2 times, most recently from fa64ee7 to 5254e2b Compare July 12, 2023 01:51
to the decision `doc/decisions/0_drafts/commit_function.md`
- move to state "solutions clear"
- add new assumption
- extend the description of some possible solutions
@flo91 flo91 marked this pull request as ready for review July 12, 2023 03:48
@flo91 flo91 added needs review please review this PR and removed work in progress documentation labels Jul 12, 2023
@flo91 flo91 requested a review from markus2330 July 12, 2023 03:49
flo91 and others added 2 commits July 12, 2023 06:01
- describe the current situation in more detail
- mention the affected plugin functions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants