Regarding usability, you are going to have to keep explaining things to everyone until we make the scripts better. I would suggest
- Merge the two scripts together. If we want to be able to do the functionalities separately we can do that with command line flags. - Make the output text explain things for someone who doesn't know what .mailmap is or anything. - Even better, we can make the script have an interactive mode, which asks people basic questions and updates things automatically based on that. - Write a longer description of everything on the wiki page, and link to it from the development workflow. Actually, now that running it is required, this becomes much simpler, because the only errors will be due to git metadata mismatches for the person running the script. Before we had it on CI, it would show duplicate emails and missing lines for lots of different people, but now, it should almost always be a single person. So we can make this assumption in the output of the script. Aaron Meurer On Wed, Aug 25, 2021 at 4:54 PM Aaron Meurer <[email protected]> wrote: > > On Wed, Aug 25, 2021 at 3:26 AM Oscar Benjamin > <[email protected]> wrote: > > > > On Wed, 25 Aug 2021 at 09:22, Aaron Meurer <[email protected]> wrote: > >> > >> On Tue, Aug 24, 2021 at 6:30 PM Oscar Benjamin > >> <[email protected]> wrote: > >> > > >> > On Wed, 25 Aug 2021 at 01:07, Aaron Meurer <[email protected]> wrote: > >> > > > >> > > Are you simulating having the PR run in a "merged with master" state > >> > > when running the authors script on CI? Maybe we should update the > >> > > script itself so that it can do this. > >> > > >> > It's not a simulation. When a PR is pushed GitHub actions will make a > >> > temporary commit that merges the PR into master and then all tests run > >> > on that commit. The problem I'm referring to is that usually if you > >> > suggest that a contributor should "merge with master" in their PR the > >> > merge is in the opposite direction (merge master into the PR rather > >> > than PR into master). For most purposes this doesn't make any > >> > difference because the final state of the changes to files is the > >> > same. However the authors script looks through the topological order > >> > of the commits and that is not the same in both situations. That's why > >> > currently a rebase is needed: before the PR can be merged the > >> > topological order needs to correspond to the chronological order in > >> > which PRs are merged (but before the PRs are merged the *eventual* > >> > chronological order is not actually known). > >> > >> Right, the point is that the script currently doesn't give the same > >> results in a branch vs. that branch after being merged into master (or > >> rebased on top of master). But I think we could modify it so that it > >> does do this. I don't know if there's a fancy git command we can do, > >> but I think the simplest way would be to clone the repo into a temp > >> directory and merge the branch into master before running the git log > >> --topo-sort. > >> > >> But even so, I don't think this will solve the merge conflict problem > >> which, unless I am mistaken, will happen just from multiple people > >> adding names to the bottom of the file. > > > > > > Yes, the merge conflict issue comes from insisting that many PRs need to > > make changes to the end of the same file. A simple fix is to make it so > > that the changes are not at the end e.g. by listing the names in > > alphabetical order. > > > > The bin/authors_update.py script has a load of code to carefully get the > > order right so presumably at some point someone thought that the ordering > > was important. > > Honestly, the reason is that the AUTHORS file was always in that > order, and I always tried to maintain the ordering when updating it > manually. So when we wrote the script, we made it so that it gave the > same ordering that was already there. See the discussion at > https://github.com/sympy/sympy/pull/10239. So the ordering isn't that > important. IMO alphabetical is fine. The only real issue with it is > that there's no reasonable way to automatically sort by last name > (which not everyone even has), so we have to just sort by first name > (we do a fake last name sort in the release notes, but we should > probably just do first name there too). We can maybe have some flag to > the script that prints the authors in the current order in case anyone > is ever interested in that. > > > > > I wonder how other projects do this... > > In my experience most other projects aren't great about maintaining a > .mailmap file. Maybe once we make the tooling here work well we can > separate it from SymPy so that other projects can make use of it as > well. > > One alternative I know of is to use the authors activity from rever > https://regro.github.io/rever-docs/api/activities/authors.html. This > generates AUTHORS and .mailmap from a yaml file. I believe the code > for it is based on the SymPy scripts. I'm not sure if that level of > indirection actually solves the problem or makes it worse, however. > > Aaron Meurer > > > > > > > Oscar > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "sympy" group. > > To unsubscribe from this group and stop receiving emails from it, send an > > email to [email protected]. > > To view this discussion on the web visit > > https://groups.google.com/d/msgid/sympy/CAHVvXxR16TftRd2mRXrq6Nsdra5S%2BSYbvhtgGX%2Bf9Wmx4vAE9w%40mail.gmail.com. -- You received this message because you are subscribed to the Google Groups "sympy" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/sympy/CAKgW%3D6Jq%3DB%3D8XgJUkyMmWmhQcVKSLpfnxU2o6MkyVM27S9F5Ag%40mail.gmail.com.
