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

[cli] fix ruff and unit tests for --excludes #25379

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions python_modules/dagster/dagster/_cli/project.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import os
import sys
from typing import NamedTuple, Optional, Sequence
from typing import NamedTuple, Optional, Sequence, Tuple, Union

import click
import requests
Expand Down Expand Up @@ -117,7 +117,15 @@ def scaffold_repository_command(name: str):
type=click.STRING,
help="Name of the new Dagster code location",
)
def scaffold_code_location_command(name: str):
@click.option(
"--excludes",
multiple=True,
type=click.STRING,
default=[],
help="Exclude file patterns from the project template",
)
def scaffold_code_location_command(name: str, excludes: Union[Tuple, list]):
excludes = list(excludes)
dir_abspath = os.path.abspath(name)
if os.path.isdir(dir_abspath) and os.path.exists(dir_abspath):
click.echo(
Expand All @@ -126,7 +134,7 @@ def scaffold_code_location_command(name: str):
)
sys.exit(1)

generate_code_location(dir_abspath)
generate_code_location(dir_abspath, excludes)
click.echo(_styled_success_statement(name, dir_abspath))


Expand Down
1 change: 1 addition & 0 deletions python_modules/dagster/dagster/_generate/download.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
"deploy_ecs",
"deploy_k8s",
"development_to_production",
"etl_tutorial",
"feature_graph_backed_assets",
"project_analytics",
"project_dagster_university_start",
Expand Down
27 changes: 20 additions & 7 deletions python_modules/dagster/dagster/_generate/generate.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,13 @@

from dagster.version import __version__ as dagster_version

IGNORE_PATTERN_LIST = ["__pycache__", ".pytest_cache", "*.egg-info", ".DS_Store", "tox.ini"]
IGNORE_PATTERN_LIST: list[str] = [
"__pycache__",
".pytest_cache",
"*.egg-info",
".DS_Store",
"tox.ini",
]


def generate_repository(path: str):
Expand All @@ -26,7 +32,7 @@ def generate_repository(path: str):
click.echo(f"Generated files for Dagster repository in {path}.")


def generate_code_location(path: str):
def generate_code_location(path: str, excludes: list = []):
CODE_LOCATION_NAME_PLACEHOLDER = "CODE_LOCATION_NAME_PLACEHOLDER"

click.echo(f"Creating a Dagster code location at {path}.")
Expand All @@ -38,6 +44,7 @@ def generate_code_location(path: str):
project_template_path=os.path.join(
os.path.dirname(__file__), "templates", CODE_LOCATION_NAME_PLACEHOLDER
),
excludes=excludes,
)

click.echo(f"Generated files for Dagster code location in {path}.")
Expand Down Expand Up @@ -65,7 +72,11 @@ def generate_project(path: str):


def _generate_files_from_template(
path: str, name_placeholder: str, project_template_path: str, skip_mkdir: bool = False
path: str,
name_placeholder: str,
project_template_path: str,
skip_mkdir: bool = False,
excludes: list[str] = [],
):
normalized_path = os.path.normpath(path)
code_location_name = os.path.basename(normalized_path).replace("-", "_")
Expand All @@ -76,11 +87,13 @@ def _generate_files_from_template(
loader = jinja2.FileSystemLoader(searchpath=project_template_path)
env = jinja2.Environment(loader=loader)

# merge custom skip_files with the default list
excludes = IGNORE_PATTERN_LIST + excludes
for root, dirs, files in os.walk(project_template_path):
# For each subdirectory in the source template, create a subdirectory in the destination.
for dirname in dirs:
src_dir_path = os.path.join(root, dirname)
if _should_skip_file(src_dir_path):
if _should_skip_file(src_dir_path, excludes):
continue

src_relative_dir_path = os.path.relpath(src_dir_path, project_template_path)
Expand All @@ -96,7 +109,7 @@ def _generate_files_from_template(
# For each file in the source template, render a file in the destination.
for filename in files:
src_file_path = os.path.join(root, filename)
if _should_skip_file(src_file_path):
if _should_skip_file(src_file_path, excludes):
continue

src_relative_file_path = os.path.relpath(src_file_path, project_template_path)
Expand Down Expand Up @@ -124,13 +137,13 @@ def _generate_files_from_template(
f.write("\n")


def _should_skip_file(path):
def _should_skip_file(path: str, excludes: list[str] = IGNORE_PATTERN_LIST):
"""Given a file path `path` in a source template, returns whether or not the file should be skipped
when generating destination files.

Technically, `path` could also be a directory path that should be skipped.
"""
for pattern in IGNORE_PATTERN_LIST:
for pattern in excludes:
if pattern in path:
return True

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,17 @@ dependencies = [

[project.optional-dependencies]
dev = [
"dagster-webserver",
"dagster-webserver",
"pytest",
]

[build-system]
requires = ["setuptools"]
build-backend = "setuptools.build_meta"

[tool.setuptools.packages.find]
exclude=["{{ code_location_name }}_tests"]

[tool.dagster]
module_name = "{{ code_location_name }}.definitions"
code_location_name = "{{ code_location_name }}"
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,29 @@ def test_scaffold_code_location_command_succeeds():
assert origins[0].loadable_target_origin.module_name == "my_dagster_code.definitions"


def test_scaffold_code_location_command_exclude_succeeds():
runner = CliRunner()
with runner.isolated_filesystem():
result = runner.invoke(
scaffold_code_location_command,
# NOTE: wildcard is not working, need to fix
[
"--name",
"my_dagster_code",
"--excludes",
"setup.cfg",
"--excludes",
"setup.py",
Comment on lines +88 to +91
Copy link
Contributor

Choose a reason for hiding this comment

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

In my testing it looks like there is an implicit wildcard before and after the exclude strings.

Suggested change
"--excludes",
"setup.cfg",
"--excludes",
"setup.py",
"--excludes",
"setup",

"--excludes",
"tests",
],
)
assert result.exit_code == 0
assert not os.path.exists("my_dagster_code/setup.cfg")
assert not os.path.exists("my_dagster_code/setup.py")
assert not os.path.exists("my_dagster_code/tests/")


def test_from_example_command_fails_when_example_not_available():
runner = CliRunner()
with runner.isolated_filesystem():
Expand Down