-
Notifications
You must be signed in to change notification settings - Fork 20
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
Checkstyle and github workflow action #20
Checkstyle and github workflow action #20
Conversation
Signed-off-by: Rick Poleshuck <RickPoleshuck@gmail.com>
Signed-off-by: Rick Poleshuck <RickPoleshuck@gmail.com>
There's a lot going on in this PR Rick, so it's hard to review - can you summarize what's going on in the comments? I put my specific questions into my code review here. |
with: | ||
java-version: '11' | ||
distribution: 'adopt' | ||
- run: mvn --batch-mode --update-snapshots -Pprod verify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm new to Github Actions - does this actually deploy somewhere?
<property name="message" value="No printing to console, use a logger."/> | ||
</module> | ||
</module> | ||
</module> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with this particular linter, but it should be good to add.
<suppressions> | ||
<!-- io.swagger package is generated, don't run checks --> | ||
<suppress files="[\\/]io[\\/]swagger[\\/]" checks="[a-zA-Z0-9]*"/> | ||
</suppressions> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
return qr; | ||
} | ||
|
||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the intent of these changes?
public QueryResult<Client> getAllClientsOfAttorney(String attorneyId) { | ||
QueryResult<Client> qr = db.query(new QueryBuilder(eq("attorneyId", attorneyId)).build(), Client.class); | ||
return qr; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the intent of these changes?
String healthURL = (uriInfo.getAbsolutePath() + "/v1/health").replaceAll("(?<!http:)\\/\\/", "/"); | ||
String exampleURL = (uriInfo.getAbsolutePath() + "/v1/example").replaceAll("(?<!http:)\\/\\/", "/"); | ||
return Response.ok("{\"health\":\"" + healthURL + "\",\"example\":\"" + exampleURL + "\"}").build(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just a simple formatting change then?
Right now I am recommending a rewrite of the Aggregator. If that is
approved, this PR is moot. Let's circle back after tomorrow's meeting.
…On Wed, 2021-08-25 at 04:37 -0700, Ann Marie Fred wrote:
There's a lot going on in this PR Rick, so it's hard to review - can
you either break it up into a couple of PRs or summarize what's going
on in the comments?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
There are 3 parts to this PR.
|
👋 Hi! This pull request has been marked stale due to inactivity. If no further activity occurs, it will automatically be closed. |
👋 Hi! This pull request has been marked stale due to inactivity. If no further activity occurs, it will automatically be closed. |
@kossiefu who should be the reviewer here from MS? |
@RickPoleshuck, @upkarlidder do we still want to move forward with this change? It aligns with what the ms_cohort haas been working on. We can build on top of it. |
No description provided.