Skip to content

docs: add CONTRIBUTING.md file#816

Merged
odudex merged 1 commit into
selfcustody:developfrom
qlrd:docs/contrib-guidelines
May 25, 2026
Merged

docs: add CONTRIBUTING.md file#816
odudex merged 1 commit into
selfcustody:developfrom
qlrd:docs/contrib-guidelines

Conversation

@qlrd
Copy link
Copy Markdown
Member

@qlrd qlrd commented Jan 7, 2026

What is this PR for?

CONTRIBUTING.md is a important file that guides new developers through the standards built by krux team through years. This isn't a monad and could be changed at time to time.

Changes made to:

  • Code
  • Tests
  • Docs
  • CHANGELOG

Did you build the code and tested on device?

  • Yes, build and tested on

What is the purpose of this pull request?

  • Bug fix
  • New feature
  • Docs update
  • Other

@qlrd qlrd force-pushed the docs/contrib-guidelines branch from 5955e70 to ef98a0d Compare January 7, 2026 15:02
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.30%. Comparing base (9ef5803) to head (db7641d).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #816   +/-   ##
========================================
  Coverage    97.30%   97.30%           
========================================
  Files           83       83           
  Lines        10798    10798           
========================================
  Hits         10507    10507           
  Misses         291      291           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@qlrd qlrd force-pushed the docs/contrib-guidelines branch 5 times, most recently from b5e9334 to 1137f7d Compare January 7, 2026 15:12
@qlrd qlrd marked this pull request as ready for review January 7, 2026 15:22
@qlrd qlrd force-pushed the docs/contrib-guidelines branch 2 times, most recently from 3cf9dd5 to 71f346a Compare January 8, 2026 23:08
Copy link
Copy Markdown
Member

@odudex odudex left a comment

Choose a reason for hiding this comment

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

Needs a few changes

Comment thread CONTRIBUTING.md Outdated
Comment thread CONTRIBUTING.md Outdated
Comment thread CONTRIBUTING.md Outdated
Comment thread CONTRIBUTING.md Outdated
@qlrd qlrd force-pushed the docs/contrib-guidelines branch 2 times, most recently from c9202cf to 902a07d Compare April 5, 2026 13:38
@qlrd qlrd marked this pull request as draft April 5, 2026 13:40
@qlrd qlrd force-pushed the docs/contrib-guidelines branch 5 times, most recently from 60423df to 0b07342 Compare April 5, 2026 15:01
@qlrd qlrd force-pushed the docs/contrib-guidelines branch from 0b07342 to f4a2cec Compare April 15, 2026 02:02
@qlrd qlrd marked this pull request as ready for review April 15, 2026 02:07
@qlrd qlrd requested a review from odudex April 15, 2026 02:08
@qlrd qlrd force-pushed the docs/contrib-guidelines branch from f4a2cec to 67cf800 Compare April 15, 2026 14:50
Comment thread CONTRIBUTING.md Outdated
Comment thread CONTRIBUTING.md Outdated
Comment thread CONTRIBUTING.md Outdated
Comment thread CONTRIBUTING.md Outdated
Comment thread CONTRIBUTING.md Outdated
@qlrd qlrd marked this pull request as draft April 15, 2026 21:23
@qlrd qlrd force-pushed the docs/contrib-guidelines branch 4 times, most recently from b0584cb to 7fd1f55 Compare April 15, 2026 21:42
@qlrd qlrd requested a review from odudex April 16, 2026 13:15
Comment thread CONTRIBUTING.md
Comment thread CONTRIBUTING.md
@qlrd qlrd force-pushed the docs/contrib-guidelines branch 4 times, most recently from d0b417c to 6e9b220 Compare May 11, 2026 12:48
Comment thread CONTRIBUTING.md Outdated

#### Minimum Supported Python Version

The Minimum Supported Python Version is **3.12.0** (enforced by our CI).
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@odudex, thought if is worth to down or maintain 3.12, wdyt?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO the source of this information is pyproject.toml. No need to replicate (and maintain) it elsewhere. You can add a link to the file mentioning user can learn about all python dependencies there.
No need to lower the requirement version. This is a decision the would require testing anyway.
Let project.toml speak for itself

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Removed and added a #863 to address that, wdyt?

@qlrd qlrd marked this pull request as ready for review May 11, 2026 12:53
@qlrd qlrd requested a review from odudex May 11, 2026 12:53
@odudex
Copy link
Copy Markdown
Member

odudex commented May 11, 2026

Please ensure everything CONTRIBUTING.md states is true now. Code style, what is enforced by CI, what's not. It must be coherent with project's current status.
If new requirements and tools will later be adopted, then CONTRIBUTING can be adjusted accordingly.

@odudex
Copy link
Copy Markdown
Member

odudex commented May 11, 2026

  1. Python version is stated three different ways across the repo, and CONTRIBUTING adds a fourth conflict.
  • CONTRIBUTING.md:119 says minimum is 3.12.0 and "enforced by our CI".
  • pyproject.toml:32 requires python = "^3.12.3".
  • .github/workflows/tests.yml actually runs CI on 3.11 for every job (lint, pylint, tests, coverage).
  • The claim "enforced by our CI" is therefore false. CI doesn't enforce 3.12 at all.
  1. Markdown linting is not enforced.
  • CONTRIBUTING.md:215 says rules "are enforced by CI, both for python and markdown code" and lists markdown rules at line 278.
  • No markdown linter exists in .github/workflows/, and no .markdownlint*/.pymarkdown* config is present. There's an in-flight branch ci/pymarkdownlnt, but it hasn't landed.
  1. Conventional-commit types list is incomplete.
  • CONTRIBUTING.md:130-142 lists 8 types.
  • .github/workflows/conventional-commits.yml allows feat,fix,docs,style,refactor,test,i18n,ci,chore,git.
  • i18n and git are missing from the doc despite being CI-allowed.
  1. Fork instructions are wrong.
    • CONTRIBUTING.md:67 shows git clone git@github.com:selfcustody/krux.git under "Fork Repository". That clones upstream, not a fork. There's no instruction to actually fork via GitHub or add the upstream remote.
  2. CONTRIBUTING.md:260, 263 uses instanceof(...) — that's JavaScript. Python is isinstance(...). The "good example" wouldn't run.
  3. CONTRIBUTING.md:226-231 Python skeleton is indented 3 / 6 / 12 spaces — black (which the project uses) will rewrite to 4-space. Example contradicts the formatter that CI runs.

@qlrd qlrd force-pushed the docs/contrib-guidelines branch 2 times, most recently from 416cc4c to 8960644 Compare May 12, 2026 11:54
@qlrd
Copy link
Copy Markdown
Member Author

qlrd commented May 20, 2026

The claim "enforced by our CI" is therefore false. CI doesn't enforce 3.12 at all.

Removed

CONTRIBUTING.md:260, 263 uses instanceof(...) — that's JavaScript. Python is isinstance(...). The "good example" wouldn't run.

Sorry, my fault

CONTRIBUTING.md:226-231 Python skeleton is indented 3 / 6 / 12 spaces — black (which the project uses) will rewrite to 4-space. Example contradicts the formatter that CI runs.

think was local markdown linter, txs

@qlrd qlrd force-pushed the docs/contrib-guidelines branch from 8960644 to 88e153f Compare May 20, 2026 21:46
Copy link
Copy Markdown
Member

@odudex odudex left a comment

Choose a reason for hiding this comment

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

utACK 88e153f4

The overall structure (workflow → review →
conventions → testing → release) is the right shape. The following first
three instructions are blocking, the rest are nits.

Blocking

  1. Broken link to SECURITY.md. The release section points contributors
    with security issues to ./SECURITY.md, but that file doesn't exist in
    the repo. Could we either add a stub SECURITY.md (even a short one
    pointing at a contact address or GitHub's security advisory flow) or
    change the link to wherever we actually want reports to go?

  2. Fork / clone instructions are contradictory. The first block tells
    the contributor to fork and clone their fork; the "Stay update with the
    upstream" subsection then says to instead clone the upstream as origin
    and add the fork as a named remote. That's the opposite of the
    conventional setup and contradicts the step right above it. I'd suggest
    picking one approach — typically: clone your fork as origin, then
    git remote add upstream git@github.com:selfcustody/krux.git.

  3. Workflow list is truncated. The numbered list reads:

    1. Fork Repository
    2. Create topic branch
    3. Commit patches
    

    The two steps that actually produce a contribution (push to fork, open
    PR, then address review) are missing. Also "the workflow is as follows:"
    appears twice back-to-back.

Non-blocking suggestions

  1. git as a commit type. The doc claims Conventional Commits 1.0.0,
    then lists git with the description "used when changing some git
    stuffs". That isn't part of the spec and the description is hard to
    act on. Could we either drop it or define exactly what it covers
    (.gitignore, .gitattributes?) — though those would normally fit
    under chore.

  2. CHANGELOG placement. The Added CONTRIBUTING.md guidelines line
    landed in ### Other Bug Fixes and Improvements. By the doc's own
    taxonomy that's a docs change, not a fix/improvement. Either a new
    ### Documentation subsection or just dropping the changelog entry
    (the commit itself is self-describing) would feel more consistent.

  3. nACK definition is ambiguous. Line currently reads:

    Approach nACK: "I do (not) agree with the approach of this change"

    The (not) reads as a typo. nACK is unambiguously "I do not agree" —
    suggest dropping the parens.

  4. Unreferenced footnote. [^1]: https://withblue.ink/...how-and-why-to-sign-git-commits.html
    sits at the bottom of the file but no [^1] is referenced anywhere in
    the body. Either link it from a paragraph (a short note on signing
    commits would be a natural spot) or remove it.

  5. poetry run poe discoverability. "See more with poetry run poe"
    prints usage rather than the task list — poetry run poe --help (or
    pointing readers at the [tool.poe.tasks] block in pyproject.toml)
    is what we probably want here.

Changelog

We use CHANGELOG mostly to report firmware changes to users, sometimes simulator, but not project structure and development related changes.

Copy / typos

  • Commit body: "a important file" → "an important file"; "isn't a
    monad" — I think the intended word is monolith (a monad is a
    category-theory construct; "not set in stone" is the meaning here).
  • "Stay update with the upstream" → "Stay up to date with upstream".
  • "as did in previously step" → "as in the previous step".
  • "mostly addtion/fix" → "addition".
  • The Python docstring example shows """Some awesome comment""" as a
    placeholder; a one-line realistic summary would teach more.
  • The doc enforces 80 chars per line for markdown, but several lines in
    the file itself exceed that (e.g. the contributors-list line). Running
    the project's markdown linter against it would catch these.

Smaller structural notes

  • The "Named branches" section is mostly about commit hygiene (atomic
    commits, no mixing formatting with code). Splitting into "Branch
    naming" and "Commit hygiene" would match the content.
  • tACK / utACK are defined under both "Conceptual Review" and "Code
    Review". Defining them once and cross-referencing would shorten the
    section.
  • The Code Review example's <diff> block is non-standard and won't
    render the way the prose implies — a fenced ```diff block would be
    cleaner.

What's good

The Conventional Commits list (with the git caveat) is concrete and
actionable, the ACK / nACK / tACK / utACK vocabulary borrowed from the
Bitcoin Core review culture fits this project's audience well, and
linking the poe lint / poe format tasks ties the doc to commands
people will actually run. Nice first cut overall — happy to help with
any of the above if useful.

@qlrd qlrd marked this pull request as draft May 23, 2026 18:59
@qlrd
Copy link
Copy Markdown
Member Author

qlrd commented May 23, 2026

Addressed Blocking issues (and removed CHANGELOG change)

git as a commit type.

already addressed

Approach nACK: "I do (not) agree with the approach of this change"
The (not) reads as a typo. nACK is unambiguously "I do not agree" — suggest dropping the parens.

This is how is in Floresta

typically: clone your fork as origin, then
git remote add upstream git@github.com:selfcustody/krux.git.

I had problems with the submodules --init --recursive using upstream instead origin. AFAIK the git need it to be named as origin (if have another way, great). So i think i will put only the first.

I think we could open sub-issues for non-blocking and address them on follow-ups, sounds good?

@qlrd qlrd force-pushed the docs/contrib-guidelines branch 2 times, most recently from b755a5a to 3392158 Compare May 23, 2026 19:02
@qlrd qlrd marked this pull request as ready for review May 23, 2026 19:03
`CONTRIBUTING.md` is a important file that guides new developers through
the standards built by krux team through years. This isn't a monad and
could be changed at time to time.
@qlrd qlrd force-pushed the docs/contrib-guidelines branch from 3392158 to db7641d Compare May 23, 2026 19:10
Copy link
Copy Markdown

@bitcoisas bitcoisas left a comment

Choose a reason for hiding this comment

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

Strong cACK

After reading the full text and comments, just some nits/observations:

  • CHANGELOG not mentioned: The PR template has a CHANGELOG checkbox,
    but the guide never tells contributors when they should update it.
    Worth adding a line under the Open PR section, IMO.
  • Commit signing: @qlrd suggested commits should follow the same signing
    behaviour as release assets, but this seems unresolved. Worth settling
    before merge. Is signing a requirement, recommendation, or out of scope?

Comment thread CONTRIBUTING.md
`docs` are accompanied in the PR);
- `fix`: bug fix (see if it could break changes -- `!`, it's recommended that
`test` and `docs` are accompanied);
- `git`: used when changing some git stuffs;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

stuff is uncountable in English

@odudex odudex merged commit db7641d into selfcustody:develop May 25, 2026
8 checks passed
@odudex
Copy link
Copy Markdown
Member

odudex commented May 25, 2026

The commit from this PR was merged, with a follow up with fixes and more conciseness.

@qlrd qlrd deleted the docs/contrib-guidelines branch May 26, 2026 11:52
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.

3 participants