Skip to content

Add script to query for contributors between two releases#253

Merged
wilzbach merged 3 commits intodlang:masterfrom
wilzbach:contributors
Dec 8, 2017
Merged

Add script to query for contributors between two releases#253
wilzbach merged 3 commits intodlang:masterfrom
wilzbach:contributors

Conversation

@wilzbach
Copy link
Copy Markdown
Contributor

@wilzbach wilzbach commented Jul 22, 2017

I don't know yet how we are going to integrate this on dlang.org, but this script should be more than flexible enough to accompany many possible scenarios.

Ideas:

  • Add to the bottom of each changelog
  • Separate page for each changelog (linked from changelog) (that's how the Rails people or the Rust people handle this)
  • All contributors page (the tool is more than fast enough, s.t. we could run this on every deploy)

@dlang-bot
Copy link
Copy Markdown
Contributor

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

contributors.d Outdated

File(gitComitMappingFile)
.byLineCopy
// ignore empty lines and comments, but allow `#ponce`
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.

Huh?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah there's a line in the .commitmap that starts like this, but thinking about it it makes more sense to use an extra whitespace for it.

@CyberShadow
Copy link
Copy Markdown
Member

I have created a small .commitmap

That sounds like something that ought to be done via the GitHub API (or just taken out of the git history).

@CyberShadow
Copy link
Copy Markdown
Member

What is this .commitmap file BTW? Some existing convention? Couldn't find anything about it.

@wilzbach
Copy link
Copy Markdown
Contributor Author

What is this .commitmap file BTW? Some existing convention? Couldn't find anything about it.

Yeah I will use mailmap on the next iteration.

@andralex
Copy link
Copy Markdown
Member

lgtm

@MartinNowak
Copy link
Copy Markdown
Member

Copy link
Copy Markdown
Member

@MartinNowak MartinNowak left a comment

Choose a reason for hiding this comment

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

Might just be me, but this looks as if it could be done in about 10-20 lines of bash.

contributors.d Outdated
line.skipOver(Author);
return line.readUser;
})
.filter!(a => a.name != "The Dlang Bot")
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.

Maybe you'd rather want to filter out merges, --no-merges.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There's already a flag to change to this behavior. However, I wasn't sure whether we really want to exclude merges by default.

@wilzbach wilzbach force-pushed the contributors branch 3 times, most recently from d863be3 to c301e82 Compare September 11, 2017 00:01
@wilzbach
Copy link
Copy Markdown
Contributor Author

Might just be me, but this looks as if it could be done in about 10-20 lines of bash.

Probably, but then (1) you always run into some special behavior of bash or things that are tricky and (2) (and this is the main reason) doing it in D allows us more flexibility as I am still not sure where we will use/display this information

Yeah I will use mailmap on the next iteration.

Done. This should be good as an initial script. The .mailmap file might require some tweaking in the future, but this is something that is probably unavoidable with new users starting to contribute. At least now we use the standard git format.

@wilzbach
Copy link
Copy Markdown
Contributor Author

wilzbach commented Dec 4, 2017

Ping @CyberShadow @andralex @ZombineDev - can we move forward with this?
It would be awesome to finally start generating a list of all contributors for each release.

contributors.d Outdated
return line.readUser;
})
.filter!(a => a.name != "The Dlang Bot")
.each!(author => authors ~= author);
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.

Nit, unnecessary each. Instead you can do authors ~= p.stdout.....filter!(...);

Copy link
Copy Markdown
Member

@CyberShadow CyberShadow left a comment

Choose a reason for hiding this comment

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

Looks good, seems to work as intended.

contributors.d Outdated
import std.uni : sicmp;
return authors
.sort!((a, b) => sicmp(a.name, b.name) < 0)
.release
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.

.release seems unnecessary here

contributors.d Outdated
auto p = pipeProcess(cmd, Redirect.stdout);
scope(exit) enforce(wait(p.pid) == 0, "Failed to execute '%(%s %)'.".format(cmd));

static immutable Author = "Author: ";
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.

Instead of parsing the default git log output, you can just ask it to print the data in the format you want. You probably want --pretty=format:%aN.

contributors.d Outdated
email: ps.dropOne.front.filter!(a => a != '"').until('>').to!string
};
return author;
}
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.

This function is unnecessary if you use git log's format option.

@wilzbach
Copy link
Copy Markdown
Contributor Author

wilzbach commented Dec 5, 2017

Instead of parsing the default git log output, you can just ask it to print the data in the format you want. You probably want --pretty=format:%aN.

Well initially I did the mapping myself, before I figured that it's probably a lot better to use .mailmap.
While you have a fair point that parsing the git log is now mostly obsolete, there are still some points why I don't want to remove it:

  • output in the git format (--format git) or csv format (--format csv)
  • mapping with Gravatars (possible future)
  • mapping with GitHub usernames (possible future)

OTOH I don't really care that much about it. What do you think?

@CyberShadow
Copy link
Copy Markdown
Member

Sorry, I think there's a misunderstanding. I meant, you're expending a non-trivial amount of code/logic for parsing the default git log output format. Whereas, we could use the --pretty=format:... switch to git log to ask it to give us only the data we want in the format that's easiest for us to parse.

@wilzbach
Copy link
Copy Markdown
Contributor Author

wilzbach commented Dec 8, 2017

Somehow my message got lost due to the great WiFi on German trains....
Anyhow:

Whereas, we could use the --pretty=format:... switch to git log to ask it to give us only the data we want in the format that's easiest for us to parse.

"+3 −16" - can't argue much about this!

-> Will merge this now & look into integrating it with dlang.org

@wilzbach wilzbach merged commit 404256f into dlang:master Dec 8, 2017
@wilzbach wilzbach deleted the contributors branch December 8, 2017 11:31
Walter Waldron <WalterWaldron@users.noreply.github.com>
Yazan Dabain <yazan.dabain@arabiaweather.com> <yazan.dabain@gmail.com>
Михаил Страшун <public@dicebot.lv> <m.strashun@gmail.com>
Yao Gómez <yao.gomez@gmail.com> <yaoltzin@gmail.com>
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.

Should we remove the dlang repo specific mailmaps then?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

enforce(wait(p.pid) == 0, "Failed to execute '%(%s %)'.".format(cmd));
}

auto cmd = ["git", "-c", "mailmap.file=%s".format(config.mailmapFile), "-C", repo, "log", "--use-mailmap", "--pretty=format:%aN|%aE"];
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.

git -C only works with newer versions of git

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

git 1.8.3 is an ancient dinosaur (more than four years old) 😉

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.

std.process functions allow you to specify a working directory anyway.

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.

5 participants