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

Paging api #19

Merged
merged 11 commits into from
Dec 9, 2016
Merged

Paging api #19

merged 11 commits into from
Dec 9, 2016

Conversation

Gawen-pjr
Copy link
Contributor

@Gawen-pjr Gawen-pjr commented Dec 7, 2016

Opening another PR. Seems I did a false maneuver integrating your comments...

Sorry for the lost remarks...

@@ -1,5 +1,5 @@
group = com.cdancy
version = 0.0.10
version = 0.0.10-pjr
Copy link
Contributor Author

@Gawen-pjr Gawen-pjr Dec 7, 2016

Choose a reason for hiding this comment

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

Could we set the version to 0.0.10-SNAPSHOT instead to enforce the fact that this version is not yet released ?

@Gawen-pjr Gawen-pjr mentioned this pull request Dec 7, 2016
@cdancy cdancy added this to the v0.0.11 milestone Dec 7, 2016
@cdancy cdancy self-assigned this Dec 7, 2016
@@ -1,5 +1,5 @@
group = com.cdancy
version = 0.0.10-pjr
version = 0.0.10-SNAPSHOT
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, it should actually be 0.0.11-SNAPSHOT, since version 0.0.10 is already released, and you targeted this PR to v0.0.11 milestone.

Test is more relevant than including parameters in expected path as param order is not significative. Also discard silently empty params.
@Gawen-pjr
Copy link
Contributor Author

Gawen-pjr commented Dec 7, 2016

Added mock and live test as expected. Mock test pass with success. For live test, I wait on Travis check

@cdancy
Copy link
Owner

cdancy commented Dec 8, 2016

@Gawen-pjr live tests are not wired together on travis. This is more due to Bitbucket and standing up an instance requires a license (I use a dev license for local development when necessary otherwise I connect to an instance we use here at work). Either way, I'll give the tests a live run tomorrow morning and see how things fare. Thanks AOT!

@cdancy cdancy merged commit 9dd09a3 into cdancy:master Dec 9, 2016
@cdancy
Copy link
Owner

cdancy commented Dec 9, 2016

Integration tests pass as expected. Thanks @Gawen-pjr !!!

Did you need a new release with this change?

@Gawen-pjr
Copy link
Contributor Author

Not yet. Still adding new features. I'll come back with a new PR soon ;-)

@Gawen-pjr Gawen-pjr deleted the paging-api branch December 9, 2016 20:16
@Gawen-pjr Gawen-pjr restored the paging-api branch December 9, 2016 20:17
@cdancy
Copy link
Owner

cdancy commented Dec 9, 2016

@Gawen-pjr very good and thanks! Have you previously worked with jclouds before? I'm assuming yes just wanted to be sure. We're a small group all things considered. I ask because I've started another project api-processor which is an attempt to factor out the core processing of jclouds (the part which turns Api's, and their methods, into something invokable) into its own project which will then eventually be used here and in a few other projects I have that use jclouds core which is a bit heavy weight. Take a peek if you're interested as I could use the help and don't come across many devs in the wild who understand this code ;)

@Gawen-pjr Gawen-pjr deleted the paging-api branch December 9, 2016 20:27
@Gawen-pjr
Copy link
Contributor Author

It was acually my first time with jclouds, but I'm quite used to play with meta-programming, so I had no problem to understand it's philosophy.

For your other project, I'll take a peek as you suggested. But I can't engage myself, as I don't know if I'll have enough time for it. I have my own business and customers to manage...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants