Skip to content

Glob based model paths #177

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

Closed
onebesky opened this issue Oct 24, 2017 · 12 comments
Closed

Glob based model paths #177

onebesky opened this issue Oct 24, 2017 · 12 comments

Comments

@onebesky
Copy link

Some projects keep models within modules right next to controllers. Current implementation of sequelize-typescript allows to specify only path to folder with models.

What do you think about adding support for the following configuration:

modelPaths: [__dirname + '/**/*.model.ts']
@RobinBuschmann
Copy link
Member

Sounds good. I think we should implement this next.

@RobinBuschmann
Copy link
Member

#24

@victorivens05
Copy link
Contributor

Hey guys.
I made some simple changes and was about to make sure they worked, before I opened a PR, when I found this discussion.
So, what will be done? Add the include option in the sequelize constructor or implement glob in the modelPaths?

@RobinBuschmann
Copy link
Member

Using modelPaths for an additional globing functionality should be fine. So that using a path to a directory with modelPaths should continue working as well.

@victorivens05 What is you suggestion?

@victorivens05
Copy link
Contributor

victorivens05 commented Oct 30, 2017

@RobinBuschmann in my tests I managed to use glob and folder together.
Using glob, if you pass a folder: c:\project\models it doesn't return anything, but there's a method called glob.hasMagic with which we can detect if the user passed a folder or a glob.
If the user passed a folder, we concatenate with \* and get every file in the folder.

It should work, unless some user has a folder called !(folder), @(folder) or [folder]

The problem right now is that I work alone, so I don't have much experience working with teams. The tests seems to be working (first time I've seen a real test coverage run, pretty sick) and I don't know if I should create a branch and submit a PR.

@RobinBuschmann
Copy link
Member

@victorivens05 Sounds good! Don't worry. Create your PR and we will have a look :) If there is something that need to be adjusted we can discuss that in the context of your PR. If you have any questions don't hesitate to ask.

@victorivens05
Copy link
Contributor

victorivens05 commented Oct 30, 2017

I just submitted the PR. I already think some stuff should be done differently. I didn't create a test, I'd like to create one, but I would need some help to figure out where and how it should be done.

The README should also be changed to reflect the new changes in the code.

@snewell92
Copy link
Contributor

snewell92 commented Oct 30, 2017

Worth mentioning from this other issue thread

I think it would be much clearer to use only paths for modelPaths option and no globs. Using globs in include, exclude with an absolute path should work.

It might be a good idea to leave modelPaths alone, and then add include and exclude options that do exactly what the linked PR is doing for modelPaths.

I'll defer to @RobinBuschmann tho - it will be super nice to have globs either which way imo. :D (the original request in this thread wanted glob support in modelPaths tho - /shrug ).

@RobinBuschmann
Copy link
Member

@snewell92 ha, maybe I'm right with that one. We could use an option called models for passing either an array of paths, of globs or of model references to simplify and mark modelPaths as deprecated... or include as initially intended.

@victorivens05

@victorivens05
Copy link
Contributor

In my opinion, a breaking change should only be made when there is a great benefit attached to it or there is no other way to solve a problem.

The way the PR was submitted, nothing will have to change and there will be a new (altough small) feature.

@victorivens05
Copy link
Contributor

I think this PR #181 closes this issue.

@RobinBuschmann
Copy link
Member

Closed due to #181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants