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

Jb/initial lib #1

Merged
merged 12 commits into from
Aug 16, 2018
Merged

Jb/initial lib #1

merged 12 commits into from
Aug 16, 2018

Conversation

jbarr21
Copy link
Collaborator

@jbarr21 jbarr21 commented Aug 16, 2018

This PR contains the initial creation of this library. It contains the forked, cleaned up, and namespaced copy of resources-poet in addition to the api, core, and top-level modules of the stylist library. A sample providers module and library module are also included, but will be used in a sample app in a followup PR.


def build = [
androidPlugin : "com.android.tools.build:gradle:3.1.0",
buildToolsVersion : "27.0.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't be necessary to specify this anymore

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

buildToolsVersion is for the sample app

Copy link
Contributor

Choose a reason for hiding this comment

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

Still shouldn't be necessary, AGP sets it on its own

]

def build = [
androidPlugin : "com.android.tools.build:gradle:3.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

3.1.4 is the latest stable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

*/

def versions = [
kotlin : "1.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not 1.2.60?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

had copied from artist. 49ae0be

gradleAptPlugin : "net.ltgt.gradle:gradle-apt-plugin:0.15",
gradlePluginsUrl : "https://plugins.gradle.org/m2/",
minSdkVersion : 16,
targetSdkVersion : 27
Copy link
Contributor

Choose a reason for hiding this comment

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

libraries shouldn't specify this (unless it's for the sample)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for the sample

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok 👍

def test = [
compileTesting : "com.google.testing.compile:compile-testing:0.11",
junit : 'junit:junit:4.12',
robolectric : "org.robolectric:android-all:8.1.0-robolectric-4402310",
Copy link
Contributor

Choose a reason for hiding this comment

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

This version looks...weird?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed in 49ae0be. don't think i'll need it

@@ -0,0 +1,16 @@
apply plugin: "org.jetbrains.kotlin.jvm"

sourceCompatibility = JavaVersion.VERSION_1_7
Copy link
Contributor

Choose a reason for hiding this comment

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

another place that could be java 8

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

check.dependsOn 'checkstyle'

dependencies {
api deps.support.appcompat
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: implementation since it's an app

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

task checkstyle(type: Checkstyle) {
source 'src'
include '**/*.java'
exclude '**/gen/**'
Copy link
Contributor

Choose a reason for hiding this comment

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

crazy idea - might be worth enabling this on generated code to make sure you're generating valid/well-formed xml?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

between aapt2 and integration tests to be added in #4 we should already be sufficiently covered

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant more to be readable

apply plugin: 'net.ltgt.apt'
apply plugin: "java-library"

sourceCompatibility = JavaVersion.VERSION_1_7
Copy link
Contributor

Choose a reason for hiding this comment

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

can be java 8

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

);

private StyleItemGroup textSizes = new StyleItemGroup(
new StyleItem("textSizeSmall", "12dp"),
Copy link
Contributor

Choose a reason for hiding this comment

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

I kind think it'd be really good to make the API more pedantic like java/kotlinpoet do. making a sealed class of the types (value, attr, or reference) and make the API more idiomatic to read

DimensionItem("textSizeSmall", 12, Dimen.DP) // Value (can be a nicer form to achieve the next line)
DimensionItem("textSizeSmall", ValueReference("12dp")) // Value
DimensionItem("textSizeSmall", DimensionReference("textSizeTooSmall")) // Reference - to `@dimen/textSizeTooSmall`. Could take an optional namespace arg too
DimensionItem("textSizeSmall", AttrReference("textSizeTooSmall")) // Attr - to `?textSizeTooSmall`. Could take an optional namespace arg too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Going to investigate this more in #7

Copy link
Contributor

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

LGTM, but definitely keen to see where #7 goes

@jbarr21 jbarr21 merged commit ee51b3d into master Aug 16, 2018
@jbarr21 jbarr21 deleted the jb/initial-lib branch August 17, 2018 00:38
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