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

Contributing guide #138

Merged
merged 1 commit into from
May 19, 2017
Merged

Contributing guide #138

merged 1 commit into from
May 19, 2017

Conversation

BenFradet
Copy link
Contributor

No description provided.

}
```

## Wrapper
Copy link
Contributor Author

@BenFradet BenFradet May 18, 2017

Choose a reason for hiding this comment

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

I don't know what happened but the Wrapper section is offset almost centered.

@codecov-io
Copy link

codecov-io commented May 18, 2017

Codecov Report

Merging #138 into master will increase coverage by 0.17%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
+ Coverage      88%   88.17%   +0.17%     
==========================================
  Files          36       36              
  Lines         550      558       +8     
  Branches        2        2              
==========================================
+ Hits          484      492       +8     
  Misses         66       66
Impacted Files Coverage Δ
...b4s/shared/src/main/scala/github4s/api/Users.scala 100% <0%> (ø) ⬆️
...src/main/scala/github4s/free/algebra/UserOps.scala 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa439da...43004ba. Read the comment docs.

Copy link
Member

@juanpedromoreno juanpedromoreno left a comment

Choose a reason for hiding this comment

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

Outstanding work, thanks so much!

I've added some minor comments though. I'd also add a Coproduct section, for those cases people want to add a new API.

@@ -0,0 +1,386 @@
---
layout: docs
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if it makes sense to put this awesome contributing guide in the microsite main page, using page layout. We did it in cats in this way: http://typelevel.org/cats/contributing.html

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, that way we avoid cluttering the documentation too much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we move it to a CONTRIBUTING.md at the root of the repo as in cats?

Copy link
Member

Choose a reason for hiding this comment

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

We could, however, it's not mandatory. If we want to move it to the root I'm fine with that. Nevertheless, I'd separate it from the current CONTRIBUTING.md file in the root. What do you think about DEV_GUIDE.md or something similar? (afterward, when it's copied to the microsite territory we can rename it on the fly).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with leaving it where it is for now.

```bash
export GITHUB4S_ACCESS_TOKEN=aaaa
```

Copy link
Member

Choose a reason for hiding this comment

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

I'd add a link indicating how generate this personal access token

## Integration tests

The integration tests are grouped by API in [github4s.integration package][integ-pkg]. As a result,
we'll be writing our tests in [GHReposSpec][repos-integ-spec-scala]:
Copy link
Member

Choose a reason for hiding this comment

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

Let's add that we only want integration tests for read-only operations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the funny thing is that I was planning on adding it but completely forgot about it 😄

@BenFradet BenFradet force-pushed the bf-contributing branch 2 times, most recently from 6c27c40 to 7d4962c Compare May 19, 2017 17:30
Copy link
Member

@juanpedromoreno juanpedromoreno left a comment

Choose a reason for hiding this comment

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

Looks great!

giphy 3

@juanpedromoreno juanpedromoreno merged commit dd94b49 into master May 19, 2017
@juanpedromoreno juanpedromoreno deleted the bf-contributing branch May 19, 2017 18:18
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