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

Adopting V3 API in routes #1194

Closed
wants to merge 7 commits into from

Conversation

radoslav-tomov
Copy link
Contributor

@radoslav-tomov radoslav-tomov commented Sep 7, 2023

Change adopts V3 API in the following operations:

  • check
  • deleteOrphanedRoutes
  • delete

Copy link
Contributor

@anthonydahanne anthonydahanne left a comment

Choose a reason for hiding this comment

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

please keep reformatting in a standalone commit; it makes it really hard to review if changes and reformatting (please configure your IDE not to reformat every single class you updated) are bundled together; thank you!

Actually even better: please stick to the existing code style; some IDEs like IntelliJ recognize the current layout and do not try to reduce lines to a specific length

@@ -30,7 +30,7 @@
import org.cloudfoundry.client.v2.routes.DeleteRouteResponse;
Copy link
Contributor

Choose a reason for hiding this comment

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

unused import

Copy link
Contributor

Choose a reason for hiding this comment

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

also line 61: import static org.cloudfoundry.util.tuple.TupleUtils.predicate;

import org.cloudfoundry.client.v2.jobs.GetJobResponse;
import org.cloudfoundry.client.v2.jobs.JobEntity;

import org.cloudfoundry.client.v3.jobs.GetJobRequest;
Copy link
Contributor

Choose a reason for hiding this comment

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

lots of unused imports here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anthonydahanne Thanks for checking it out. Sure, will do.

@radoslav-tomov
Copy link
Contributor Author

@anthonydahanne Apologies for the formatting. I've been using the VS Code and it's default styling is a bit off. I've removed the unused imports and reformatted via the standard Eclipse google java style.

@anthonydahanne
Copy link
Contributor

@anthonydahanne Apologies for the formatting. I've been using the VS Code and it's default styling is a bit off. I've removed the unused imports and reformatted via the standard Eclipse google java style.

Thanks for the effort, but unfortunately, a lot of unchanged code is still reformatted.

We need to enforce formatting using CI to make sure we won't be having this discussion anymore.

I'll let you know when we have it.

Thanks for your patience

@radoslav-tomov
Copy link
Contributor Author

@anthonydahanne Ok. Thanks.

anthonydahanne added a commit to anthonydahanne/cf-java-client that referenced this pull request Feb 13, 2024
@anthonydahanne anthonydahanne mentioned this pull request Feb 13, 2024
anthonydahanne added a commit to anthonydahanne/cf-java-client that referenced this pull request Feb 13, 2024
@beyhan
Copy link
Member

beyhan commented Feb 15, 2024

This could be closed now because #1217 introduced this change.

@pivotal-david-osullivan
Copy link
Contributor

Closed via #1217

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