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

Fixed Issue #358: rename class job #380

Conversation

@Violet-XiaoWeiHuang
Copy link
Contributor

commented Mar 4, 2019

class Job has been renamed to JobResponse.
Please review

@sshah-wework

This comment has been minimized.

Copy link
Collaborator

commented Mar 4, 2019

Hi @Violet-XiaoWeiHuang ! Thanks for this, looks like you may have missed changing Job to JobResponse in src/main/java/marquez/api/models/JobsResponse.java.

You can also run './gradew build` locally to ensure the project builds correctly.

@wslulciuc
Copy link
Member

left a comment

@Violet-XiaoWeiHuang, this is a great start! But, there are a couple things we'll need to fix before merging your changes:

  • We only want to rename the Job class in the package marquez.api.models to JobResponse. The PR also renames the Job class in marquez.service.models, which is not what we want. Mind reverting renaming marquez.service.models.Job?
  • I dont't think your branch is in sync with master? The class JobDao is introducing changes that have been removed in #344 . You can learn how to sync your fork here: https://help.github.com/en/articles/syncing-a-fork

Once we work through these changes, we can merge your PR :)

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