Skip to content

Implementation of RCML Java Builder SDK. #23

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

Open
wants to merge 39 commits into
base: master
Choose a base branch
from

Conversation

rlimonta
Copy link

No description provided.

@jaimecasero
Copy link
Contributor

please ignore target dir to prevent .class files to go into SCM

@rlimonta
Copy link
Author

@jaimecasero We have a /target/ dir on .gitignore file

Copy link
Contributor

@gsaslis gsaslis left a comment

Choose a reason for hiding this comment

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

Hi @rlimonta !

Thanks for taking the time to contribute this!

Could I please ask for some changes, so that we can make this easier to review and therefore give it a higher chance of getting it merged?

  • Let's try to separate the commits that delete the .class and other maven artifact files under target in another PR, as a kind of hotfix. This should be dead simple for us to merge.
  • If you could then cherry-pick the rest into a separate PR.
  • Could you also include some documentation (maybe a couple of lines in the README.md about what this PR adds to the SDK ?

As a final note, I'd also point out that writing more meaningful commit messages (that explain the why, not the what ), really shows some <3 to the person reviewing your code, so it also means they'll more happily spend their time going through your PR and merging it in. ; )

Thanks in advance!

rlimonta and others added 2 commits December 13, 2017 10:54
Example of using the RCML library.
@rlimonta
Copy link
Author

@jaimecasero and @gsaslis I implemented the requested improvements. Best regards!

@gsaslis
Copy link
Contributor

gsaslis commented Feb 6, 2018

@rlimonta thanks for the extra work on this and adding some docs.

Are you also planning to proceed with my suggestion to split this PR into 2 separate ones?

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.

4 participants