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

arangorizeSchema #15

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

arangorizeSchema #15

wants to merge 16 commits into from

Conversation

ltwlf
Copy link

@ltwlf ltwlf commented Aug 29, 2020

Great project! I wanted to created exactly the same. I'm going to use it for a software product and would be glad to contribute.

I this PR I added an arango schema generator. The generators adds the all directives and can also create the db, doc & edge collections and indexes.

I also updated typescript, arangojs and other packages. With the current setting I had to remove node 8 support.

The tests are integrations tests and start and stop an arangodb docker container. So running the tests requires docker.

Looking forward to your feedback.

Best wishes,
Christian

@a-type
Copy link
Owner

a-type commented Dec 9, 2020

I apologize for not responding sooner, I must have missed the notification for this PR. This is certainly a significant contribution and I really appreciate you taking the time. I'm going to merge another smaller fix in a moment, do a bit of cleanup on the main branch, then I'll take a look.

It's been a minute since I worked on this library or with Arango in general, so I may need some time to review this and test it out.

@a-type
Copy link
Owner

a-type commented Dec 9, 2020

Ok, I updated this repo with som QOL improvements. Running tests in CI now. Will review in a moment.

Copy link
Owner

@a-type a-type left a comment

Choose a reason for hiding this comment

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

This is a really great streamlining feature and I'd be happy to include it!

Two things to discuss before approval:

  • I think I'd prefer tests which utilize integration with Docker to run separately from standard unit tests. Can you add a new npm script test-integration which runs Jest with a separate config, and move the test suite into __integration_tests__? In fact there's already an old Jest config in the root dir which will accommodate this.
  • How attached are you to the name? I'd prefer makeArangoSchema to be consistent with the makeSchema naming convention, since this function is replacing that usage I think that would be intuitive.

@mysticaltech
Copy link

@ltwlf Any updates on this, such a contribution would be really appreciated! Counting on you, please 🙏🏻

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