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

Allow 'migrations' folder as subfolder of sql, not sql/sql_server #38

Open
schuemie opened this issue Sep 6, 2023 · 2 comments
Open
Assignees
Labels
enhancement New feature or request

Comments

@schuemie
Copy link
Member

schuemie commented Sep 6, 2023

In recent versions of SqlRender it is allowed to put your OhdsiSql in the inst/sql folder instead of the inst/sql/sql_server folder, and loadRenderTranslateSql() will still work. The main reasons are

  1. Techincally, it's OhdsiSql, not SQL Server SQL,
  2. Looks prettier.
  3. SqlRender has proven to be very capable of handling cross-platform idiosyncrasies, so most (if not all) packages have no need for platform-specific SQL.

(Note that plafform-specific code can still be placed in the appropriately named subfolders. It is just that 99.9% of SQL is OhdsiSql, so why not put it up a level?)

Currently ResultModelManager throw an error if I put the 'migrations' folder in the 'sql' folder. A simple extra lookup when looking for migration SQL would solve this. Could this be added?

@azimov azimov self-assigned this Sep 8, 2023
@azimov azimov added the enhancement New feature or request label Sep 8, 2023
@azimov
Copy link
Collaborator

azimov commented Sep 12, 2023

Because of the support for flat folder structures outside of a package this issue is not as straightforward to resolve as I would have liked, I will add this in a future version when I have a bit more time to test it fully.

@schuemie
Copy link
Member Author

Just a reminder that it would still be nice to have this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants