-
-
Notifications
You must be signed in to change notification settings - Fork 292
Fix pyproject.toml formatting when appending alembic config #1684
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -554,13 +554,57 @@ def run_env(self) -> None: | |
def env_py_location(self) -> str: | ||
return str(Path(self.dir, "env.py")) | ||
|
||
def _append_template(self, src: Path, dest: Path, **kw: Any) -> None: | ||
with util.status( | ||
f"Appending to existing {dest.absolute()}", | ||
**self.messaging_opts, | ||
): | ||
|
||
def _append_template( | ||
self, template_path: Path, output_path: Union[str, Path], **kw: Any | ||
) -> None: | ||
""" | ||
Append content from a Mako template to an existing file. | ||
|
||
Special handling for TOML files to ensure proper formatting between | ||
sections, addressing issue #1679. | ||
|
||
Args: | ||
template_path: Path to the template file | ||
output_path: Path to the output file to append to | ||
**kw: Keyword arguments for template rendering | ||
""" | ||
output_path = Path(output_path) | ||
|
||
# For TOML files, we need special handling to ensure proper spacing | ||
if output_path.suffix.lower() == '.toml' and output_path.exists(): | ||
# Read existing content | ||
with open(output_path, 'r', encoding=self.output_encoding) as f: | ||
existing_content = f.read() | ||
|
||
# Check if we need to add newlines before appending | ||
existing_content = existing_content.rstrip() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is not really a "check", this just truncates what was there so that it can add two newlines unconditionally |
||
|
||
# Save the modified content back | ||
with open(output_path, 'w', encoding=self.output_encoding) as f: | ||
f.write(existing_content) | ||
|
||
# TOML convention: ensure 2 newlines between top-level sections | ||
# The template should start with [tool.alembic], so we need proper spacing | ||
if existing_content: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why did we open and rewrite the file if no existing content? |
||
f.write('\n\n') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK but what if the template_to_file command fails? now we have two newlines that will keep getting bumped each time (although I guess the strip() and rewrite-the-whole-file approach fixes that, but I dont want to rewrite the whole file). Id rather this be all within one file.open() and once we have the output from the template in #1680 we add the two newlines in the single open |
||
|
||
# Now append the template content | ||
util.template_to_file( | ||
template_path, | ||
output_path, | ||
self.output_encoding, | ||
append=True, | ||
**kw | ||
) | ||
else: | ||
# For non-TOML files or new files, just use template_to_file directly | ||
util.template_to_file( | ||
src, dest, self.output_encoding, append=True, **kw | ||
template_path, | ||
output_path, | ||
self.output_encoding, | ||
append=True, | ||
**kw | ||
) | ||
|
||
def _generate_template(self, src: Path, dest: Path, **kw: Any) -> None: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
"""Tests for pyproject.toml template functionality.""" | ||
import os | ||
import tempfile | ||
from pathlib import Path | ||
|
||
from alembic import command | ||
from alembic.config import Config | ||
from alembic.testing import TestBase | ||
|
||
|
||
class TestPyprojectTemplate(TestBase): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good test but this should be in tests/test_command.py->CommandLineTest->test_init_file_pyproject_formatting , it should have four cases: one where there's no pyproject.toml, one where there's an empty pyproject.toml, one where there's an existing pyproject.toml with no trailing whitespace (maybe even a fifth test, where there's one trailing newline, not two), one where there's a pyproject.toml with existing trailing whitespace. |
||
"""Test the pyproject template, particularly issue #1679.""" | ||
|
||
def test_init_pyproject_existing_file_formatting(self): | ||
"""Test that appending to existing pyproject.toml maintains proper spacing.""" | ||
with tempfile.TemporaryDirectory() as tmpdir: | ||
os.chdir(tmpdir) | ||
|
||
# Create existing pyproject.toml | ||
pyproject_path = Path("pyproject.toml") | ||
pyproject_path.write_text( | ||
'[tool.black]\ntarget-version = [\'py37\']\n\n' | ||
'[tool.mypy]\npython_version = "3.10"' | ||
) | ||
|
||
# Run init | ||
config = Config("pyproject.toml") | ||
command.init(config, "alembic", template="pyproject") | ||
|
||
# Check result | ||
result = pyproject_path.read_text() | ||
|
||
# Verify no concatenation (issue #1679) | ||
assert 'python_version = "3.10"[tool.alembic]' not in result | ||
|
||
# Verify proper spacing | ||
assert '\n\n[tool.alembic]' in result | ||
|
||
# Verify content preserved | ||
assert '[tool.black]' in result | ||
assert '[tool.mypy]' in result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we've read the content here. Can we just look to see if it already ends with "\n\n" ? that's what we did originally at #1680 why rewrite the file