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

[WIP] Cleanup build.gradle defaults not used #663

Closed
wants to merge 2 commits into from

Conversation

brodycj
Copy link
Contributor

@brodycj brodycj commented Feb 10, 2019

  • remove defaultTargetSdkVersion which was never used
  • comment out defaultBuildToolsVersion & defaultCompileSdkVersion for now

This is a quick-fix solution for #658.

- remove defaultTargetSdkVersion which was never used
- comment out defaultBuildToolsVersion & defaultCompileSdkVersion
  with notes that these values should be defined and used
  someday in the future

Resolves apache#658
Resolves apache#631
@codecov-io
Copy link

codecov-io commented Feb 10, 2019

Codecov Report

Merging #663 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #663   +/-   ##
=======================================
  Coverage   64.66%   64.66%           
=======================================
  Files          18       18           
  Lines        1817     1817           
=======================================
  Hits         1175     1175           
  Misses        642      642

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 a6f30b6...6c09ea8. Read the comment docs.

@oliversalzburg
Copy link

LGTM

Copy link
Member

@janpio janpio left a comment

Choose a reason for hiding this comment

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

FUTURE TBD is not something that should be in the code but an issue on GitHub.

Are we sure those are not needed anywhere? Why were they here before? Did you investigate why/when they were added?

@brodycj
Copy link
Contributor Author

brodycj commented Feb 12, 2019

FUTURE TBD is not something that should be in the code but an issue on GitHub.

Reworded, with blank lines inserted as well. I did already raise issues concerning the defaults which should be used someday in the future.

Are we sure those are not needed anywhere? Why were they here before? Did you investigate why/when they were added?

I would say yes. I did grep through the Gradle files to hopefully ensure that I got it right. Details about where and how the defaults were introduced are in issue #658.

Copy link
Member

@janpio janpio left a comment

Choose a reason for hiding this comment

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

Ok.

@janpio
Copy link
Member

janpio commented Feb 12, 2019

(I didn't actually want to approve this, as I have no idea what it could break. But I can't find a way to remove the approval without requesting changes - which I also don't want to do. Please someone else approve this before merging.)

@brodycj brodycj changed the title Cleanup build.gradle defaults not used [WIP] Cleanup build.gradle defaults not used Feb 12, 2019
@brodycj
Copy link
Contributor Author

brodycj commented Feb 12, 2019

I am closing this one for now.

As I said in #655 (comment) we may still need to deal with target SDK version.

Issue #658 (default SDK values never used) remains unsolved at this point.

@brodycj brodycj closed this Feb 12, 2019
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.

None yet

4 participants