search

JuliaStats / Distributions.jl

A Julia package for probability distributions and associated functions.
Other
1.11k stars 416 forks source link

Add JuliaFormatter to workflow #1401

Open st-- opened 3 years ago

st-- commented 3 years ago

yeah we should add JuliaFormatter to this. It would be good to merge as many PRs as possible before because it will create conflicts all over

Originally posted by @matbesancon in https://github.com/JuliaStats/Distributions.jl/issues/1283#issuecomment-934149214

NB- the following workflow should only have a minimal amount of pain involved in each remaining open PR, assuming format(".") on master is committed as a single commit tagged format_commit:

  1. git checkout feature_branch
  2. git merge format_commit^ (no conflicts, or actual content conflicts that must be resolved should be handled here)
  3. julia -e "using JuliaFormatter; format("."); git commit -am "PR format"
  4. git merge format_commit; git checkout --ours .; git commit -a (we know the only diff is the formatting, so it's safe to blanket-accept the local branch side)
  5. git merge master (should apply cleanly now)

Of course, PRs that would be straightforward to merge should probably be merged first. Here's a (not necessarily 100% correct nor complete) list of the ones that seem like they mostly need a "click merge":

devmotion commented 3 years ago

Seems not completely striaghtforward to update PRs (in particular for new contributors or people who are not very experienced with git), so I think we should not rush this. I think it would be nice to adopt a code style at some point but IMO it is not a top priority (many/most packages are fine without one) and the code looks quite reasonable already without enforcing a specific style. I think it would be better to take the time first to go through the existing PRs and drafts and finish or close them.

st-- commented 3 years ago

I think it would be better to take the time first to go through the existing PRs and drafts and finish or close them.

In principle I'd agree, but I'm just wondering how likely is it that's actually going to happen ?

Seems not completely striaghtforward to update PRs (in particular for new contributors or people who are not very experienced with git),

How many people allow "edits by maintainers"? It might be easier to just do it on behalf of the PR openers - then they;ll just have to git pull?

devmotion commented 3 years ago

In principle I'd agree, but I'm just wondering how likely is it that's actually going to happen ?

I don't know, I check old issues and PRs from time to time at least. Even more so now that you bumped some of the PRs :smile: Also previously sometimes I finished PRs or opened alternatives such that the bug fixes or features become available.

How many people allow "edits by maintainers"?

I don't know either but apparently not everyone. I merged master into a PR locally today but was not allowed to push the changes. Seems I have to open a new PR.

st-- commented 3 years ago

@devmotion @matbesancon how about joining Closember ? :)

devmotion commented 3 years ago

I don't understand the goal of it, I think number of closed issues or PRs is a terrible metric.

I guess I might misunderstand something but it mainly reminds me of the Github bot that closes issues after some time automatically. IMHO this is a really bad idea. I also think one should not relate number of issues and PRs to well-being of maintainers. No maintainer is obliged to address issues or review or merge PRs, and it can't be demanded from them by any user. This blog post summarizes my view on issues and maintainers of opensource projects quite well: https://drewdevault.com/2021/10/26/stalebot.html

That being said, it's always great if someone opens a PR that fixes an issue. I think, however, most open PRs were reviewed but are not merged yet because additional changes were requested. If a PR gets lost, it's good to bump it after while.

st-- commented 3 years ago

Well, you were both talking about wanting to close PRs:

It would be good to merge as many PRs as possible before

I think it would be better to take the time first to go through the existing PRs and drafts and finish or close them.

Which did strike me as leading to a game of whack-a-mole (given that new ones crop up all the time), and I just thought a concerted effort (for which something like Closember might provide some motivation) might help with that.

Regarding your comment:

No maintainer is obliged to address issues or review or merge PRs.

I agree that you're not obliged to add features (or even fix any issues). Anyone in the community can do that. But I do think by being a maintainer you take up the obligation to review and merge PRs - or to close them, if you think they aren't fit for reviewing&merging!

The blog post you linked describes

A space for the community to work, rather than an action item for you to deal with personally. It gives people a place to record additional information, and, ultimately, put together a pull request for you to review.

Only maintainers can review or merge PRs. I appreciate this is something for which you're just volunteering your time! But if the nominal maintainers didn't review or merge PRs, then does that not leave a project effectively unmaintained? Leaving the community unable to actually contribute to it.

Just speaking from my personal experience, I am keen to contribute bugfixes & features that improve compatibility between packages. But it's also me contributing my time, and it's pretty demotivating if the maintainers then don't actually respond to it.