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

Read schema subfolders #230

Merged
merged 5 commits into from Oct 14, 2019
Merged

Read schema subfolders #230

merged 5 commits into from Oct 14, 2019

Conversation

lksilva
Copy link
Contributor

@lksilva lksilva commented Oct 10, 2019

Check List

  • Tests has been run in packages where changes made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Issue Reference this PR resolves

[For example #12]

Description of Changes Made (if issue reference is not provided)

[Description goes here]

@paveltiunov
Copy link
Member

@lksilva Hey Lucas! Thanks for contributing this! Looks great! The only thing: fileName is used to be local relative to schemaPath. We should save this behavior as there's some stuff that depends on it.

@lksilva
Copy link
Contributor Author

lksilva commented Oct 13, 2019

Do you have any suggestion to resolve this issue? That I would be very happy in implement.

I apply this suggestion because we need this solution here at work

Copy link
Member

@paveltiunov paveltiunov left a comment

Choose a reason for hiding this comment

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

@lksilva Yep. Sure! Here's what you'd need to be adjusted.

packages/cubejs-server-core/core/FileRepository.js Outdated Show resolved Hide resolved
packages/cubejs-server-core/core/FileRepository.js Outdated Show resolved Hide resolved
packages/cubejs-server-core/core/FileRepository.js Outdated Show resolved Hide resolved
@lksilva
Copy link
Contributor Author

lksilva commented Oct 14, 2019

I made the changes that you requested and I'm thinking about do test to cover FileRepository

@paveltiunov
Copy link
Member

paveltiunov commented Oct 14, 2019

@lksilva Looks great! Could you please rebase your branch, resolve conflicts and fix linter errors? After that we're good to merge.

Test would be great! You can add it in this PR or create separate.

# Conflicts:
#	packages/cubejs-server-core/core/FileRepository.js
@lksilva
Copy link
Contributor Author

lksilva commented Oct 14, 2019

I prefer apply in another PR because I will take a little time for this, can it be like this?

@paveltiunov
Copy link
Member

@lksilva Yep. Sure! Sounds good!

@paveltiunov paveltiunov merged commit aa736b1 into cube-js:master Oct 14, 2019
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.

None yet

2 participants