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 Optional Validate Lambda to Delegates #4

Merged
merged 1 commit into from Aug 8, 2019

Conversation

kevincianfarini
Copy link
Contributor

I'm making this pull request as a proof of concept for validation. I've only modified one delegate so that we might have a conversation about the implementation. Afterwards I could apply the decision we've come to on the other applicable delegates.

:)

@kevincianfarini kevincianfarini changed the title Add Optional Validate Lambda to stringPref Add Optional Validate Lambda to Delegates May 10, 2019
@kevincianfarini
Copy link
Contributor Author

Also, it was brought up that this should probably live in a separate module: krate-validation. While I do see the benefit of that, it seems like there are also several drawbacks.

Namely, should the krate-validation module have knowledge of the krate-gson module? If not, do we include a validation overload directly in the krate-gson module?

This is my first time working in a multi module android project, and I'm not quite sure how to approach this scenario. Any guidance would be appreciated.

@kevincianfarini kevincianfarini force-pushed the feature/validation branch 2 times, most recently from d51af79 to 3c3d6d5 Compare May 12, 2019 15:05
@kevincianfarini
Copy link
Contributor Author

Is anyone available to review this PR?

@zsmb13
Copy link
Collaborator

zsmb13 commented May 28, 2019

Hey, I am aware of this PR, but unfortunately I still don't have time to work on / discuss this feature. I'll try to get to it sometime next month, but I can not make promises on the timeline.

If you really need this feature in the library, you should be able to add it using extensions within your own project for the time being.

@zsmb13 zsmb13 self-assigned this May 28, 2019
@kevincianfarini
Copy link
Contributor Author

Hi, hope this isn't a bother but I'm just pinging to keep this PR alive :)

@kevincianfarini
Copy link
Contributor Author

hey, just pinging again. Any feedback on how I could implement this on other properties?

@kevincianfarini kevincianfarini force-pushed the feature/validation branch 2 times, most recently from b1de3cc to 61bb62a Compare August 3, 2019 17:50
@kevincianfarini
Copy link
Contributor Author

kevincianfarini commented Aug 4, 2019

I've pushed some updates to include the rest of the delegates in the standard krate module. I can't seem to be able to build and run tests though after rebasing my changes on the latest.

Getting this error.

Could not get unknown property 'ossrhUsername' for NexusStagingExtension(serverUrl:https://oss.sonatype.org/service/local/, username:null, password:null, packageGroup:hu.autsoft, stagingProfileId:caceeedcae039c, numberOfRetries:20, delayBetweenRetriesInMillis:2000, repositoryDescription:Automatically released/promoted with gradle-nexus-staging-plugin!, stagingRepositoryId:property(class java.lang.String, undefined)) of type io.codearte.gradle.nexus.NexusStagingExtension.

@zsmb13
Copy link
Collaborator

zsmb13 commented Aug 5, 2019

With the library's MavenCentral publication woes sorted out, I can finally get around to this PR (sorry for the looong wait!).

I really like the implementation, I'll try to merge it and clean it up a bit sometime this week. I'll also take a look at the overly aggressive publication scripts that are messing up your build.

@kevincianfarini
Copy link
Contributor Author

I'll try and merge it and clean it up a but sometime this week.

Would you mind leaving any comments on clean up here? Part of the reason I did this PR is to look for constructive criticism so that I can improve :)

@zsmb13
Copy link
Collaborator

zsmb13 commented Aug 6, 2019

@kevincianfarini I've left review comments, and I've also fixed the Gradle sync issue on the dev branch, so that the project will sync correctly without having the publication variables set.

@zsmb13 zsmb13 changed the base branch from master to dev August 6, 2019 18:17
@kevincianfarini
Copy link
Contributor Author

updated :)

@zsmb13 zsmb13 merged commit 79dea9f into ZenitechSoftware:dev Aug 8, 2019
@zsmb13
Copy link
Collaborator

zsmb13 commented Aug 8, 2019

Merged, cleaned up a bit, and heading towards release right now as 0.2.0!

Thank you for your contribution and especially your patience. Do you have a Twitter handle that you'd like to get a shout-out to?

@kevincianfarini
Copy link
Contributor Author

kevincianfarini commented Aug 8, 2019

@zsmb13 I don't have a twitter handle. Just doing this to learn :)

If you work on any other projects that could benefit from a contributor, let me know!

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

2 participants