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

Bound {job} to JobName #385

Merged
merged 8 commits into from Mar 6, 2019

Conversation

@pbrahmbhatt3
Copy link
Contributor

commented Mar 6, 2019

Fix375
Bound {job} in the path variable to JobName in JobResource.java file.

@pbrahmbhatt3

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Hi @wslulciuc, I am new to the pull request process. Can you please guide me through the process?

@wslulciuc

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

Sure thing. Have you looked over our contributing guide? CONTRIBUTING.md

@pbrahmbhatt3

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

For some reason I cannot run ./gradlew test. Also, once I have made the changes according to the document I have to write a test. Can you please give me an idea about what kind of test? If you can give a for instance it would be great.

@wslulciuc

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

For some reason I cannot run ./gradlew test.

I'd make sure you're running ./gradlew test at the project root. Mind providing the error you are seeing?

Also, once I have made the changes according to the document I have to write a test.

Writing a test depends on the change. For a new feature, or in the case of simply adding logic to existing code, there may not be proper code overage. So, it's important tests are included along with the PR.

In the case of your changes, we have existing tests. Meaning, once you update how we bind {job}, they'll be tests that fail and it's important to get those passing again as part of your PR.

Priyam Brahmbhatt added 3 commits Mar 6, 2019
@codecov

This comment has been minimized.

Copy link

commented Mar 6, 2019

Codecov Report

Merging #385 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #385      +/-   ##
===========================================
+ Coverage     77.15%   77.2%   +0.04%     
  Complexity      396     396              
===========================================
  Files            91      91              
  Lines          1042    1044       +2     
  Branches         48      48              
===========================================
+ Hits            804     806       +2     
  Misses          173     173              
  Partials         65      65
Impacted Files Coverage Δ Complexity Δ
...c/main/java/marquez/api/resources/JobResource.java 90.47% <100%> (+0.23%) 19 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 008a7b6...427edb3. Read the comment docs.

@pbrahmbhatt3

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

@wslulciuc Thanks for the help. I have solved the errors. Can you please review the code?

@@ -69,7 +70,7 @@ public JobResource(final NamespaceService namespaceService, final JobService job
@Timed
public Response create(
@PathParam("namespace") final String namespace,
@PathParam("job") final String job,
@PathParam("job") JobName job,

This comment has been minimized.

Copy link
@wslulciuc

wslulciuc Mar 6, 2019

Member

Minor: Let's also update the method param name: JobName job => JobName jobName

@@ -25,4 +25,8 @@
@ToString
public final class JobName {
@Getter @NonNull private final String value;

public String getVal() {

This comment has been minimized.

Copy link
@wslulciuc

wslulciuc Mar 6, 2019

Member

A getter method will be generated by lombok with @Getter. So, we can remove getVal(). Please see JobNameTest for example usage.

@wslulciuc
Copy link
Member

left a comment

Great work, @pbrahmbhatt3! I left some minor comments. We can merge in your changes once they are addressed.

Priyam Brahmbhatt added 3 commits Mar 6, 2019
@pbrahmbhatt3

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

@wslulciuc I have updated my code according to your comments. Please let me know if its good to go?

@wslulciuc
Copy link
Member

left a comment

Great work, @pbrahmbhatt3!

@wslulciuc wslulciuc merged commit 33cd0d2 into MarquezProject:master Mar 6, 2019

4 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 77.15%)
Details
codecov/project 77.2% (+0.04%) compared to 008a7b6
Details
@wslulciuc wslulciuc referenced this pull request Mar 6, 2019

@pbrahmbhatt3 pbrahmbhatt3 deleted the pbrahmbhatt3:marquez1 branch Mar 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.