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

[ext] Condense dagster-ext package to one file #16321

Merged
merged 1 commit into from
Sep 6, 2023
Merged

Conversation

smackesey
Copy link
Collaborator

@smackesey smackesey commented Sep 5, 2023

Summary & Motivation

Condenses dagster-ext into a single file dagster_ext/__init__.py so it can be vendored easily.

I didn't go all the way to just dagster_ext.py so that I could leave dagster_ext/version.py and dagster_ext/py.typed. It should be just as easy to copy-paste the file contents from __init__.py.

How I Tested These Changes

Existing test suite.

@smackesey
Copy link
Collaborator Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@smackesey smackesey force-pushed the sean/ext-one-file branch 2 times, most recently from dd8276f to 9549926 Compare September 6, 2023 17:10
@github-actions
Copy link

github-actions bot commented Sep 6, 2023

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-dcjculcmu-elementl.vercel.app
https://sean-ext-one-file.core-storybook.dagster-docs.io

Built with commit 9549926.
This pull request is being automatically deployed with vercel-action

@smackesey smackesey marked this pull request as ready for review September 6, 2023 17:14
Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

not sure about that public API section. Seems like it will just become out of date without any enforcement

Comment on lines 699 to 732
# ########################
# ##### PUBLIC API
# ########################

# Defining `__all__` would be redundant for purposes of formalizing the public API, since
# type-checkers will count any non-underscored symbol defined in this module as public. However, it
# is useful to maintain a whitelist of intended-to-be-public symbols for development and
# documentation purposes.

# ExtContext as ExtContext,
# init_dagster_ext as init_dagster_ext,
# ExtContextLoader as ExtContextLoader,
# ExtBlobStoreMessageWriter as ExtBlobStoreMessageWriter,
# ExtBlobStoreMessageWriterChannel as ExtBlobStoreMessageWriterChannel,
# ExtMessageWriter as ExtMessageWriter,
# ExtMessageWriterChannel as ExtMessageWriterChannel,
# ExtParamLoader as ExtParamLoader,
# ExtDefaultContextLoader as ExtDefaultContextLoader,
# ExtDefaultMessageWriter as ExtDefaultMessageWriter,
# ExtEnvContextLoader as ExtEnvContextLoader,
# ExtS3MessageWriter as ExtS3MessageWriter,
# DAGSTER_EXT_ENV_KEYS as DAGSTER_EXT_ENV_KEYS,
# ExtContextData as ExtContextData,
# ExtDataProvenance as ExtDataProvenance,
# ExtExtras as ExtExtras,
# ExtMessage as ExtMessage,
# ExtParams as ExtParams,
# ExtPartitionKeyRange as ExtPartitionKeyRange,
# ExtTimeWindow as ExtTimeWindow,
# DagsterExtError as DagsterExtError,
# DagsterExtWarning as DagsterExtWarning,
# decode_env_var as decode_env_var,
# encode_env_var as encode_env_var,
# is_dagster_orchestration_active as is_dagster_orchestration_active,
Copy link
Member

Choose a reason for hiding this comment

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

hmmm very skeptical of the utility of this. almost certainly will fall out of date

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fair, it was more useful while I was picking out the methods to underscore than it is now, removed

@smackesey smackesey merged commit c556654 into master Sep 6, 2023
@smackesey smackesey deleted the sean/ext-one-file branch September 6, 2023 17:57
zyd14 pushed a commit to zyd14/dagster that referenced this pull request Sep 15, 2023
## Summary & Motivation

Condenses `dagster-ext` into a single file `dagster_ext/__init__.py` so
it can be vendored easily.

I didn't go all the way to just `dagster_ext.py` so that I could leave
`dagster_ext/version.py` and `dagster_ext/py.typed`. It should be just
as easy to copy-paste the file contents from `__init__.py`.

## How I Tested These Changes

Existing test suite.
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