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

Add a CONTRIBUTING.md file #145

Merged
merged 2 commits into from
Oct 7, 2016
Merged

Add a CONTRIBUTING.md file #145

merged 2 commits into from
Oct 7, 2016

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Oct 7, 2016

r? @philipc

Anything I'm missing here that you'd like to see added?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.73% when pulling ca0a0b4 on fitzgen:contributing into 49c83e3 on gimli-rs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.73% when pulling ca0a0b4 on fitzgen:contributing into 49c83e3 on gimli-rs:master.


At the time of writing we have 94% test coverage according to our coveralls.io
continuous integration. That number should generally stay the same or go up ;)
This is a bit subjective, because -.001% is just noise and doesn't matter.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you have kcov installed under linux, then you can generate the coverage results using ./coverage in the root of the repository, and view them at target/kcov/index.html. Otherwise you can create a pull request and view the coverage results on coveralls.io.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will add, thanks!


We aim to be the fastest DWARF library. Period.

Please provide before and after benchmark results with your pull requests. You
Copy link
Collaborator

Choose a reason for hiding this comment

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

But only needed if the PR affects performance?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, we would have a CI bot to run before and after benches for us. That would be super sweet.

I don't think it hurts to encourage more benching than is strictly necessary, especially for newcomers before they've gotten a feel for how we do things here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, should we link to your dwarf-bench here? Is that something you'd like to continue building in the future? I'd like to do some more in depth cross-library benchmarking sometime (handwaves) in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

dwarf-bench isn't useful to link to yet. It's hard to use and the results probably aren't that meaningful yet. For cross-library comparisons, I think it's going to be more useful to benchmark things that solve a well-defined problem with a verifiable result, so that we know if we're comparing the same thing. So current status is that it was useful to get ballpark figures, but it's going to need a rethink to get anything more.

@fitzgen fitzgen merged commit 8f8c8b0 into gimli-rs:master Oct 7, 2016
@coveralls
Copy link

coveralls commented Oct 7, 2016

Coverage Status

Coverage remained the same at 93.73% when pulling 7cc73b0 on fitzgen:contributing into 49c83e3 on gimli-rs:master.

@fitzgen fitzgen deleted the contributing branch October 7, 2016 22:34
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.

3 participants