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 Building section to the README #407

Merged
merged 3 commits into from
Aug 7, 2020
Merged

Conversation

cortinico
Copy link
Member

📄 Context

This is related to #248, I'm adding a Building section to the README to clarify how to setup the local environment for developing the library.

📝 Changes

  • Added the Building with a mention on the ktlint pre-commit hook
  • Added the gradlew build suggested command in order to run all the tests locally
  • Disabled GradleDependency lint check as gradlew build is failing on master due to obsolete OkHTTP dependency (that's intentional).

@vbuberen
Copy link
Collaborator

vbuberen commented Aug 3, 2020

I would put it under Contributing section.
Also, as I mentioned in #248 I would like to add automatically installed pre-commit hook instead of hoping that people will read and follow.

@cortinico
Copy link
Member Author

I would put it under Contributing section.

It's actually already there (it's a h3 inside the h2 Contributing)

Copy link
Collaborator

@vbuberen vbuberen left a comment

Choose a reason for hiding this comment

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

Missed that this new section is already in Contributing section.

Looks good. Let's just add info about pre-commit hook as well.

@@ -15,6 +15,7 @@ android {
lintOptions {
warningsAsErrors true
abortOnError true
disable 'GradleDependency'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this? To not show warnings about future Gradle versions, like we have now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do we need this?

We need this as currently lint is failing on master (as we don't run it on CI) because of this.
In my change I'm suggesting to run ./gradlew build in the readme, but that command will actually fail as of now.

Either we suppress GradleDependency globally or we have to comment every dependency in the .gradle file with //noinspect GradleDependency. I generally disable this Lint check and let Gradle (or the other tools like Dependabot) take care of the dependencies.

Anyway I'm amending this change as I'll take care of the lint failures in another PR

@cortinico cortinico force-pushed the nc/build-section-in-README branch from 63369e0 to 62b9634 Compare August 3, 2020 19:01
Copy link
Collaborator

@vbuberen vbuberen left a comment

Choose a reason for hiding this comment

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

Found minor typo

Co-authored-by: Volodymyr Buberenko <vbuberen@users.noreply.github.com>
@vbuberen vbuberen merged commit 002ab9c into develop Aug 7, 2020
@vbuberen vbuberen deleted the nc/build-section-in-README branch August 7, 2020 16:20
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.

2 participants