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

break refinery into refinery_core to remove code duplication #70

Merged
merged 1 commit into from
Mar 16, 2020

Conversation

jxs
Copy link
Member

@jxs jxs commented Mar 12, 2020

Tried to come up with a way to only have 3 crates (refinery, refinery_cli and refinery_macros) and work on the new features, but can't there are a couple more functions that would be duplicate to be shared by refinery_cli and refinery_macros, therefore the decision to split refinery again into refinery_core

@jxs jxs force-pushed the break-refinery-core branch 7 times, most recently from 5d0d38b to fe882de Compare March 13, 2020 17:41
@akhilles
Copy link
Collaborator

What about putting the shared functions into a utility crate such as refinery_utils?

In general, I found that having a "shim" crate that mostly re-exports functionality from another crate isn't very ergonomic. You have to keep track of documentation and public/private APIs in 2 different places.

Basically, this is my proposal:
refinery_utils = shared functions
refinery_macros = refinery_utils + proc macros
refinery = refinery_utils + refinery_macros + migrations logic
refinery_cli = refinery_utils + refinery + cli logic

IMO, this allows each crate to have well-defined public API. But, I would also be totally fine with going in the direction of this PR.

@jxs
Copy link
Member Author

jxs commented Mar 16, 2020

What about putting the shared functions into a utility crate such as refinery_utils?

In general, I found that having a "shim" crate that mostly re-exports functionality from another crate isn't very ergonomic. You have to keep track of documentation and public/private APIs in 2 different places.

Basically, this is my proposal:
refinery_utils = shared functions
refinery_macros = refinery_utils + proc macros
refinery = refinery_utils + refinery_macros + migrations logic
refinery_cli = refinery_utils + refinery + cli logic

IMO, this allows each crate to have well-defined public API. But, I would also be totally fine with going in the direction of this PR.

yeah I also agree with that, and I tried it but it started with utils.rs which then requires error::Error which requires AppliedMigration and Migration and following that trail eventually ends up with almost all the code on refinery_utils except the macros, and therefore renaming it to refinery_core and having the shim crate which joins them

@jxs jxs merged commit d7144e5 into rust-db:master Mar 16, 2020
@zoechi
Copy link

zoechi commented Mar 17, 2020

It looks like the tests in refinery_core are not run by circle_ci
and I also can't run them locally.
I get a few errors like

error: couldn't read refinery_core/src/traits/../../tests/sql_migrations/V1-2/V1__initial.sql: No such file or directory (os error 2)
  --> refinery_core/src/traits/mod.rs:95:13
   |
95 |             include_str!("../../tests/sql_migrations/V1-2/V1__initial.sql"),
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

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

3 participants