search

tinted-theming / home

Style systems and smart build tooling for crafting high fidelity color schemes and easily using them in all your favorite apps.
MIT License
254 stars 12 forks source link

PR review guidelines #12

Closed actionless closed 9 months ago

actionless commented 2 years ago

for example, how much approvals should be for merging

joshgoebel commented 2 years ago

PR approvals also require active contributors/reviewers with familiarity with the language in use... I love code review, but it's very hard if no one has the time or makes it a priority.

Obviously reviewing themes/templates would be easier than reviewing buidlers, etc... for submitting new themes (to the unified repo) I'd imagine an issue template with a checklist would be a good start... (for submiters and reviewers alike)

actionless commented 2 years ago

let's then split this topic to 3 separate questions: 1) how many approvals needed for templates 2) ...for color-schemes 3) ...for all other infra repos

forrestli74 commented 2 years ago

I think it's related to #8. In my opinion, I hate democracy. It never gets anything done. Only project owners, and its delegations can approve, and only one approval is required.

With that being said, we can have as many reviews as we want, and the owners should request for more community feedback when fit. At the end of day, if we (as community members) can always fork if we are unhappy.

joshgoebel commented 2 years ago

Why would we need more than the approval of a single core team member/maintainer (other than perhaps major changes to the spec itself)? This quickly leads back into governance... but so far it seems like we'll likely have a small friendly "core team" for a while...

Another question (or perhaps part of what you're trying to ask?) is how much reign maintainers of individual projects hosted here should have (builders, templates). I'm happy to have PRs reviewed for node-builder, but if that were to take forever or become a convoluted process I'd rather quickly change my mind (out of necessity to get work done).

I wouldn't be super inclined to start putting set in stone review requirements on individual project maintainers unless the review time was guaranteed to be same day/next day... Another data point: as the Base16 Highlight.js Template is very closely tied to Highlight.js project if it were ever to move to the base16-project I'd expect 100% autonomy to set my own PR review policies - as to fully maintain control of the larger Highlight.js release process.

belak commented 2 years ago

I think setting explicit numbers of reviewers will end up getting tabled for now. We're not big enough to need it quite yet.

Until it becomes a problem, spec changes need to have a general consensus (I'm explicitly not setting a number of approvals here), scheme additions I don't have a strong preference on so I think anyone can merge them, but changes to existing schemes should be approved/merged by an owner (or someone on a scheme maintenance team if we add one), and template changes can be merged in by any one of the maintainers if they feel strongly about it (ping the template or project owners if there's any question).

joshgoebel commented 2 years ago

any one of the maintainers

Yeah, I'd definitely prefer starting out "lets all be friends and see how it goes" rather than default to posting armed guards around all the repos. :-) Maybe be slightly more careful with anything that auto-publishes (Go, NPM, etc) but I'm not sure we have things that auto-publish yet?

belak commented 2 years ago

Maybe be slightly more careful with anything that auto-publishes (Go, NPM, etc) but I'm not sure we have things that auto-publish yet?

I don't think we do yet, other than the schemes which get cloned directly from git (vim, emacs, etc).

joshgoebel commented 2 years ago

other than the schemes which get cloned directly from git (vim, emacs, etc).

Wait, what automation are you talking about? I'm not sure I follow.

joshgoebel commented 2 years ago

@belak How do you feel about marking main as protected and requiring PRs (as a dev workflow), not direct commits? I've almost committed directly on accident a few times now...

belak commented 2 years ago

other than the schemes which get cloned directly from git (vim, emacs, etc).

Wait, what automation are you talking about? I'm not sure I follow.

It's not really automation, just that vim schemes are generally installed via plugin managers which clone the git repo... And there's a Rube Goldberg machine that builds the emacs package so it integrates with the package manager.

@belak How do you feel about marking main as protected and requiring PRs (as a dev workflow), not direct commits? I've almost committed directly on accident a few times now...

Good idea. I'll do this later today.

belak commented 9 months ago

I'm closing this in favor of #84, where we're collecting all remaining project management related questions.