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

Move JsonSchemaValidator into its own module airbyte-validation (#234) #546

Conversation

ChristopheDuong
Copy link
Contributor

What

The JsonSchemaValidator class is in commons module so that it can be shared with the worker and few other modules, but that is not ideal because it injects these dependencies into all modules that use commons.

Ideally this class should be split out into its own module.

How

Creating a airbyte-validation module and moving its dependencies, JsonValidationException class and test into it

Copy link
Contributor

@cgardens cgardens left a comment

Choose a reason for hiding this comment

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

awesome! i might call the module airbyte-json-validation since we are not likely to add other types of validations to this modules (because the dependencies would likely be different). just the module name though, the package name you've chosen is great! what do you think?

doing that just requires changing the name of the directory that all the module code is in from airbyte-analytics => airbyte-json-validation.

@ChristopheDuong
Copy link
Contributor Author

Good point! I renamed both the directory and module names as you suggested and changed the build.gradle files that depend on it accordingly.

Is this what you had in mind?

There was also the option of renaming only the folder name, keep airbyte-validation module and tweak the settings.gradle file to specify the projectDir but that sounded potentially confusing

Copy link
Contributor

@michel-tricot michel-tricot left a comment

Choose a reason for hiding this comment

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

Cool!

@cgardens
Copy link
Contributor

this looks great!

@ChristopheDuong
Copy link
Contributor Author

FYI I merged master into my branch in order to resolve the conflicts (so it'll be easier to merge back to master afterward)...
Actually, I'm not so sure if that was a good approach now...

@cgardens
Copy link
Contributor

Yeah. it looks like a bunch of stuff got included that shouldn't be in there.

@ChristopheDuong ChristopheDuong force-pushed the chris/move_jsonschemavalidator branch from 36e2e2a to ca2c7c7 Compare October 20, 2020 08:14
@ChristopheDuong
Copy link
Contributor Author

Ok it's now fixed and in a cleaner state without conflicts for merging !!

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.

3 participants