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

docs: remove references to SQLAlchemyAsyncConfig.create_engine #2380

Merged
merged 3 commits into from
Sep 30, 2023

Conversation

lsanpablo
Copy link
Contributor

@lsanpablo lsanpablo commented Sep 28, 2023

Pull Request Checklist

  • New code has 100% test coverage
  • (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
  • (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR

Description

  • Remove references to SQLAlchemyAsyncConfig.create_engine from docs; use SQLAlchemyAsyncConfig.get_engine in examples instead.

Close Issue(s)

@lsanpablo
Copy link
Contributor Author

According to the advanced-alchemy docs, get_engine seems like the only way to get what we want in those examples.

@lsanpablo lsanpablo marked this pull request as ready for review September 28, 2023 02:23
@lsanpablo lsanpablo requested review from a team as code owners September 28, 2023 02:23
@peterschutt
Copy link
Contributor

create_engine() was part of the interface in the 2.0 release, so really should still exist.

It existed in the commit prior to the advanced-alchemy commit:

def create_engine(self) -> EngineT:
"""Return an engine. If none exists yet, create one.
Returns:
Getter that returns the engine instance used by the plugin.
"""
if self.engine_instance:
return self.engine_instance
if self.connection_string is None:
raise ImproperlyConfiguredException("One of 'connection_string' or 'engine_instance' must be provided.")
engine_config = self.engine_config_dict
try:
self.engine_instance = self.create_engine_callable(self.connection_string, **engine_config)
except TypeError:
# likely due to a dialect that doesn't support json type
del engine_config["json_deserializer"]
del engine_config["json_serializer"]
self.engine_instance = self.create_engine_callable(self.connection_string, **engine_config)
return self.engine_instance

@lsanpablo
Copy link
Contributor Author

lsanpablo commented Sep 28, 2023

I agree, create_engine should still be part of the interface. It seems like the best thing to do is to add a deprecated create_engine method which just calls get_engine and leave the docs as they are in the PR. Correct?

@cofin
Copy link
Member

cofin commented Sep 28, 2023

I agree, create_engine should still be part of the interface. It seems like the best thing to do is to add a deprecated create_engine method which just calls get_engine and leave the docs as they are in the PR. Correct?

Yes, I think this is correct. This means we should probably do this over on the advanced_alchemy side.

@guacs
Copy link
Member

guacs commented Sep 28, 2023

I agree, create_engine should still be part of the interface. It seems like the best thing to do is to add a deprecated create_engine method which just calls get_engine and leave the docs as they are in the PR. Correct?

Yes, I think this is correct. This means we should probably do this over on the advanced_alchemy side.

Wouldn't it be better to just subclass the config and add that method in litestar till v3?

@provinzkraut
Copy link
Member

Wouldn't it be better to just subclass the config and add that method in litestar till v3?

Good point. We re-export things anyway from Litestar, so we might as well just do that.

@github-actions
Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/2380

@guacs guacs merged commit 98c88e1 into litestar-org:main Sep 30, 2023
15 checks passed
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.

Docs: errors in the SQLAlchemy Repository Tutorial
5 participants