Skip to content

Expose Structure.dump_cmd/3 #423

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

Merged
merged 1 commit into from
Jul 1, 2022
Merged

Expose Structure.dump_cmd/3 #423

merged 1 commit into from
Jul 1, 2022

Conversation

maennchen
Copy link
Contributor

Follow up from #419 (comment)

[]
)

assert output =~ "INSERT INTO `schema_migrations` VALUES ("
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@josevalim I did not change anything with how the migrations work. I just took this as an example since I knew how to call it from the PR before.

@josevalim
Copy link
Member

Thank you! What about tds?

@maennchen
Copy link
Contributor Author

@josevalim Tds does not implement Ecto.Adapter.Structure

@maennchen
Copy link
Contributor Author

maennchen commented Jun 29, 2022

(It also does not look as if there's a management CLI: https://dba.stackexchange.com/questions/273158/how-can-i-dump-the-schema-of-a-sql-server-database-on-linux)

EDIT: We could implement it using SQLpackage.exe, but that should be a separate issue / PR I think.

Comment on lines 60 to 64
@callback dump_cmd(
repo_or_config :: module() | Keyword.t(),
args :: [String.t()],
opts :: Keyword.t()
) :: {output :: Collectable.t(), exit_status :: non_neg_integer()}
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, there is one last nitpick. Generally speaking, we want to make the adapter behaviour as small as possible. This means we should not receive both Repo and Config. Also, other functions in this module receive the config as the last opts, so I propose for this to be:

Suggested change
@callback dump_cmd(
repo_or_config :: module() | Keyword.t(),
args :: [String.t()],
opts :: Keyword.t()
) :: {output :: Collectable.t(), exit_status :: non_neg_integer()}
@callback dump_cmd(
args :: [String.t()],
opts :: Keyword.t(),
config :: Keyword.t()
) :: {output :: Collectable.t(), exit_status :: non_neg_integer()}

And then if you want a more convenient API which is dump_cmd(repo | config, args, opts), we can add it to Ecto.Adapters.SQL, but I think we can skip it for now and the low-level one is fine. :)

Copy link
Member

Choose a reason for hiding this comment

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

In other words, let's just change the signature here, update the code, and ship it!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@josevalim Ok, I'm on it 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@josevalim Changed

@josevalim josevalim merged commit 792a4ce into elixir-ecto:master Jul 1, 2022
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@maennchen maennchen deleted the expose_dump_cmd branch July 1, 2022 10:54
@maennchen
Copy link
Contributor Author

@josevalim Would you like me to create an issue for TDS / SqlPackage.exe support?

warmwaffles added a commit to elixir-sqlite/ecto_sqlite3 that referenced this pull request Dec 1, 2022
warmwaffles pushed a commit to elixir-sqlite/ecto_sqlite3 that referenced this pull request Dec 1, 2022
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.

2 participants