Skip to content

Conversation

@victorivens05
Copy link
Contributor

Changed the usage of FS to GLOB. If the Path isn't a glob, it's a folder
path and to work with glob, must be turned into a glob (by adding /* in
the end).
I think the check if it hasMagic should be an outside function,
chained in a map before arg.reduce.

Changed the usage of FS to GLOB. If the Path isn't a glob, it's a folder path and to work with glob, must be turned into a glob (by adding /* in the end). I think the check if it `hasMagic` should be an outside function, chained in a `map` before `arg.reduce`.
@victorivens05
Copy link
Contributor Author

@codecov-io
Copy link

codecov-io commented Oct 30, 2017

Codecov Report

Merging #181 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@ Coverage Diff @@ ## master #181 +/- ## ========================================== + Coverage 96.84% 96.85% +0.01%  ========================================== Files 100 100 Lines 918 921 +3 Branches 124 125 +1 ========================================== + Hits 889 892 +3  Misses 9 9 Partials 20 20
Impacted Files Coverage Δ
lib/services/models.ts 92.24% <100%> (+0.2%) ⬆️

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 8973931...1c4e4f8. Read the comment docs.

@RobinBuschmann
Copy link
Member

RobinBuschmann commented Oct 30, 2017

@victorivens05 You could setup testing as follows:

  • Create a sub directory in /test/models/ - for example /test/models/globs
  • Create additional sub directories and choose appropriate names matching your test cases - for example /test/models/globs/match-dir-only or /test/models/globs/match-sub-dir-files
  • Put your specs into /test/specs/models/sequelize.spec.ts -> Check if the models are loaded properly :)
@victorivens05
Copy link
Contributor Author

@RobinBuschmann I created some tests, I'm not sure if they're enough. Now how do I add them to the PR? Or I have to create another?

@RobinBuschmann
Copy link
Member

@victorivens05 Pushing you're changes to the remote branch of the PR should be enough :) Since you've created a PR from victorivens05:master, pushing to this branch should work

As suggested, I created 2 folders. In the first, all the files are already inside and should work normally. The other one has subfolders and must be matched via glob.
@RobinBuschmann
Copy link
Member

@victorivens05 Looks good 👍 I will merge it 🎉

@RobinBuschmann RobinBuschmann merged commit 57f044c into sequelize:master Oct 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants