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

proposals to revive pflag as an actively maintained repo #386

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 137 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
# Contributing guidelines

## Principles and vision

### [Roadmap to `v1.1.x`](./ROADMAP.md)

### go version

This repository supports _at least_ the 2 latest stable versions of the language.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there other major golang projects which act as dependencies to the ecosystem at large which have this sort of stability guarantee? Or are they wider than this?

Of note, Golang 1.20 was only released a year ago, which seems like a comparatively small window.

Copy link

Choose a reason for hiding this comment

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

Given that Go itself only supports the two most recent major releases with security updates, this seems appropriate to me.

ref: https://go.dev/doc/security/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My suggestion is that we commit to 2 releases (i.e. run CI on them). that doesn't mean necessarily that we ENFORCE people to upgrade (go.mod require version may lag behind)


Example: when `go1.21` is out, we may require `go1.19`

Any new language feature that is introduced should be guarded (and ideally, emulated for backward compatibility)
by go build flags, e.g. `go:build !go1.21`, `go:build go1.21`.

We don't upgrade the required version of the compiler automatically:
changes to the go version requirement in `go.mod` will occur only if a new language feature is deemed really useful.

Therefore, at any given time, older versions of the compiler might be supported. This not a promise, just a likely outcome.

Example: if we introduce generics, `go1.18` would be a requirement.

### The `pflag` compatibility promise

**Programs using older versions of `pflag` should be able to compile in the future.**

> In the spirit of the `go` language, we believe that the `golang` eco-system would benefit from a similar promise by this repository.

The required version of the compiler may however evolve.

We may use deprecation notices to advise users to move towards newer APIs.

Older APIs will continue being honored, though.

### The `pflag` promise of being a drop-in replacement of `flag`

`pflag` is a drop-in replacement for `go`'s flag package, implementing
POSIX/GNU-style --flags. That's it.

`pflag` will follow the evolution of the standard library `flag`, and continue to maintain the promise of being
a _drop-in replacement_ for `flag` (and then some of course, but at least that).

### Versioning

This package abides by `semver`.

Hence:
* `v1.0.6` : patch
* `v1.1.0` : new feature

As stated above, it is unlikely we ever issue a `v2.0.0` (breaking change).

## Pull requests

All contributions are much appreciated and welcome. There is no small contributions: additional documentation, examples,
tests, etc are all valuable additions, not just new features.

### Linting

This repository uses `github.com/golangci/golangci-lint/cmd/golangci-lint` as meta-linter, enforced by CI.
The set of rules may evolve over time. `revive` and `govet` are must-have.

> TODO: agree on the initial .golangci.yml setup, with a no-nonsense approach to linting. Code quality without nitpicking... :)
> As a started, here is a sample configuration that I use on a daily basis for personal projects (no strong agenda on this):
> [golangci-lint.yml](https://github.com/fredbi/gflag/blob/master/.golangci.yml).
Comment on lines +63 to +65
Copy link
Collaborator

Choose a reason for hiding this comment

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

Linters enabled in Viper: https://github.com/spf13/viper/blob/master/.golangci.yaml

Though I'm not sure all of them is necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sagikazarmark yeah I am going to align with the ones (also comparing with cobra)


### Testing

Pull requests should include significant unit tests.

This repository uses `github.com/stretchr/testify` framework for unit tests.

> TODO: to be discussed, what we mean by "significant", e.g. >70% test coverage or so.

### Documentation

Keeping a compelling `godoc` documentation is part of the "drop-in replacement" promise.

Any newly exposed API should come with a clear, readable description of what it does.

Testable example are alway welcome (although not required).

Conversely, internals only need comments if a particular caveat or expectation requires wording besides just code.

### Dependencies

So far, `pflag` has managed to build without _any_ external dependency.

We may indulge into _a few_ dependencies, especially test dependencies, or things like `golang.org/x/...`.

We should not need much more than that, unless we are talking about extensions (e.g. new flag types). See below.

### Commit etiquette

Please keep your commit title and description concise and to the point.

Feel free to `git rebase -i`, `git commit --amend` etc, so this repository keeps a nice and readable git history,
without merge commits or fixup rescindments.

### Contributor Licence Agreement

Contributions are subject to a CLA DCO being signed off.

> TODO: is this something that is still needed? Anyhow, check if the CLA terms are up to date.
> The Linux Foundation has introduced [EasyCLA](https://docs.linuxfoundation.org/lfx/easycla).
> I believe that `cla.assistant.io` is a bit old.
Comment on lines +104 to +106
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not a huge fan of this. I see often that folks will make a quick PR and then not follow up with the CLA (due to time or whatever) and then the PR just waits indefinitely.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@josegonzalez I am inclined to agree with the CLA being more or less just boilerplate.
However: I'd like to remain consistent with the other spf13 repos. viper has CLA check, cobra has CLA check, so should pflag.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thats fair.


### Signed commits

Commits shall be [signed](https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits).

> TODO: this requirement is debatable.
> On the one hand, it is certainly something many junior developers are perplex about.
> On the other hand, supporting so many major pieces of infrastructure is a big responsibility.
Comment on lines +112 to +114
Copy link
Collaborator

Choose a reason for hiding this comment

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

I will very often make a PR via the Github UI - for doc changes or minor cosmetic issues. I think its possible to sign in the UI, but I very frequently forget to, and then its a pain to come back and fix it locally (sometimes to the point of me just closing the PR and saying someone else can re-contribute it).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@josegonzalez this is simple sign-off (git commit -s, with email) not GPG signature.


### Linear history

Please rebase your PRs to the current master before a merging can be done. All PR's will be squashed.

## Contributed extensions

New types, great features built on top of `pflag` etc are welcome.

Considering adding those to the already rather rich collection of supported flag types is going always to be a long process.

To address that, a new repository `spf13/pflag-contrib` may host such new ideas and (possibly breaking) innovations.

That repository works under basically the same set of rules, but less stringent.

* backward-compatibility for a contributed feature remains desirable, but not required
* contributors have a freer hand regarding dependencies
* documentation & testing are needed, possibly with a lighter touch
* introduction of new go features is possible - just guard your extension with `go: build gox.yz`
to avoid breaking other contributions

> I am proposing a new git repo as I found rather impractical to maintain a nested `go.mod` in the main repo.
> Further, we most likely want to version contributed features independantly.
Comment on lines +136 to +137
Copy link
Collaborator

Choose a reason for hiding this comment

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

Contrib repos are generally hard to maintain, especially as they grow over time.

I'd probably start with a list of extensions in a markdown file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sagikazarmark point taken. Let's start with a mere document with links

104 changes: 104 additions & 0 deletions ROADMAP.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
# `pflag` roadmap
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think most of this document could become issues in the repo. Easier to track progress.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep. Exactly. Now removing that section and replacing it with issues


## Beef up the team of maintainers

3 to 5 volunteers. No less, no more.

Would be nice to open a slack or discord channel to make debate more lively.
Otherwise, we may use github discussions.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Otherwise, we may use github discussions.
Otherwise, we may use GitHub discussions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Both makes sense. We can have a channel on Gophers Slack. GH Discussions is better for archiving.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok let's try both


## Agree on the [CONTRIBUTING guidelines](./CONTRIBUTING.md)

There are quite a few debatable things there: let's find a minimal consensus
(possibly defer decisions about some details to a later stage).

## Agree on a general direction

Proposed principles:

* promise of compatibility (i.e. no breaking `v2` ever)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need to make this promise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sagikazarmark ok. Still, I'll refrain as much as I can from having to issue a breaking V2

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a good thumb rule, but as soon as you promise it, it's hard to take it back.


> Example: solving issue #384 by adding support for a default value would break compatibility.
> If we want to solve this issue, we need a new API.

* prioritize use cases from `cobra` & `viper`, acknowledged as the main venues for importing `pflag` indirectly

> Example: solving issue #250 typically comes from a `cobra` user.

* prioritize code simplification & maintenance reduction over new features

> Examples: PRs #235, #248, #332, #369, #378, #380 are representative of that effort

* deflect stream of innovation, desire to share new ideas to some dedicated `contrib` repo

> Examples: issue #246 is typically such a new flag type, requiring a specific import.


## Issue triage

There are 88 outstanding issues. Let's triage them first.

For every one of them:
* categorize: bug report, feature request/enhancement, code quality/testing, extensions ... (add new labels)
* post a comment/reply to the OP

Proposed issue labels:
* bug
Copy link
Collaborator

Choose a reason for hiding this comment

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

I usually like prefixing labels with things like kind/, resolution/, etc. It makes distinguishing between different labeling schemes easier.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok good point

* duplicate
* invalid (not a bug)
* enhancement
* won't fix
* feature request
* extension
* code quality
* investigate/need repro

Questions and the threads of answers may be used to populate a FAQ.

## PR triage

There are 70 outstanding PRs. Let's triage them first.

Identify obsolete/won't do PRs, e.g. PR #206, probably #233 & #234 as well

Prioritize merges as follows:
* bug fixes
* code quality/linting/fixups & minor refact proposals (add new labels)
* extra leeways/options (e.g. using the vararg `Option` pattern)

Same labels as for issues may be used.

At this point, isolate new features / enhancement proposals: they will be discussed later, once we get a clearer view.

New PRs may come in with more refactoring and/or improvements to testing (e.g. use testify, add fuzz test...).

## CI & tools

* [ ] Re-enact CI (github actions), with (i) tests, (ii) - later - linting, (iii) govulneck
* [ ] Simplify (no real need for the tools in `verify`)
* [ ] Enforce linting

## Repo climate

* update & revamp repo badges (code coverage, CI passed, `pkg.go.dev` etc)
* shorter README, defer details to other docs (may be published as wiki or github pages)

> I don't think we need to use github `Projects`: this is too small to justify this.
> Github `Discussions` could come in handy to collect thoughts from the community.

## Setup a dedicated repo for extensions

I suggest to create `github.com/spf13/pflag-contrib` to host such additions.

> I don't have a knack for punch lines and catchy headlines.
> There is most likely a better name to be found.

Identify PR candidates for extensions. We can manually clone or rewrite the most interesting ones.

Popular non-breaking extensions might be refitted into the main repository at some point in the future.

Examples:

* PR #247 (new type)
* PR #374 (flag validation)
* PR #114 (localization)