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

ops.py: cleanup / consistency: remove redundant docstring newline markers #1219

Closed
wants to merge 2 commits into from
Closed

ops.py: cleanup / consistency: remove redundant docstring newline markers #1219

wants to merge 2 commits into from

Conversation

jayaddison
Copy link
Contributor

Description

Potential (nitpicky) docstring cleanup discovered while preparing #1218.

Checklist

This pull request is:

  • A documentation / typographical error fix

…n method call-arguments, and make two example queries more similar
@jayaddison
Copy link
Contributor Author

The docstring here provides a sample case that could be useful for #1218 (in-progress), so I'm going to move this pull request into a draft status too.

@jayaddison jayaddison marked this pull request as draft April 10, 2023 10:42
@CaselIT
Copy link
Member

CaselIT commented Apr 10, 2023

Hi,

if we change that part we should probably format the text using black.

@CaselIT
Copy link
Member

CaselIT commented Apr 12, 2023

I'm thinking of merging this in https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/4561, so we have a single patch

@jayaddison
Copy link
Contributor Author

@CaselIT that's OK with me 👍

If you have suggestions for additional ways to provide test coverage for the script, let me know and I'll take a look.

I'm still learning about .pyi stub files (not to mention .pyx that I see elsewhere in the codebase) and this could be a good way to learn more.

@CaselIT
Copy link
Member

CaselIT commented Apr 12, 2023

not to mention .pyx that I see elsewhere in the codebase

these are cython files, and should only be on sqlalchemy side. there is no performance sensitive code on alembic :)

@jayaddison jayaddison deleted the cleanup/remove-redundant-docstring-newlines branch April 13, 2023 09:09
@jayaddison
Copy link
Contributor Author

Self-replying:

If you have suggestions for additional ways to provide test coverage for the script, let me know and I'll take a look.

Thinking about this more over the past day or so: I suppose the fact that the write_pyi.py script internally runs zimports and black on the output is a kind of self-contained test coverage.

I'm also thinking about going through some of the code samples within docstrings in the codebase to apply black code formatting to those, for consistency in the documentation.

@jayaddison
Copy link
Contributor Author

I'm also thinking about going through some of the code samples within docstrings in the codebase to apply black code formatting to those, for consistency in the documentation.

Note: doing this could require some care, because there are cases where multiline strings are used within the sample code snippets (in other words: if we're using black format, tripled double-quotes for the code snippets, then the outer docstrings in the source code -- and the write_pyi.py script -- should perhaps use tripled single-quoted strings).

I'm experimenting with this locally to see whether it produces sensible results.

@CaselIT
Copy link
Member

CaselIT commented Apr 13, 2023

by multiline strings you mean a single text that spans multiple python line, but that does not include \n or instead something that's enclosed in ''' to have multiple \n in the text?

@jayaddison
Copy link
Contributor Author

by multiline strings you mean a single text that spans multiple python line, but that does not include \n or instead something that's enclosed in ''' to have multiple \n in the text?

The second one, I think. For example:

conn.execute(text('''
create table foo (
id integer not null primary key,
old_data varchar,
x integer
)'''))
conn.execute(text('''
create table bar (
data varchar
)'''))

If reformatting those using black formatting, then the double-quotes become single-quotes, and so the outer docstring quotes can be flipped to single-quotes to accommodate that.

@CaselIT
Copy link
Member

CaselIT commented Apr 13, 2023

It's maybe simple in these case to format with black then re-convert """ to ''' so we avoid issues with the doc strings that sometimes are '''

sqlalchemy-bot pushed a commit that referenced this pull request Apr 25, 2023
### Description
This is a pedantic/consistency follow-up from #1219: that change applied some `black` formatting to two code snippets, and this change applies that formatting to the remaining snippets in the codebase.

For each snippet, I extracted the code and applied formatting using `black` v23.1.0, then placed the results back into the source.

In one case there was an associated 'output' block, and in that case I re-ran the snippet code to update the results of that output too (this included a [change-in-output](https://github.com/sqlalchemy/alembic/compare/main...openculinary:alembic:docstrings/snippet-format-consistency?expand=1#diff-bf4756660cdb31ee8566a2cff72526671356c38a725195723ce0e65e6c11e6cfR124) thanks to a [bugfix since the snippet was written](bc6971a), by the looks of it).

### Checklist

This pull request is:

- [x] A documentation / typographical error fix

Closes: #1220
Pull-request: #1220
Pull-request-sha: cd65a45

Change-Id: I6758445633c364c8fb2f4d8376d83607430a36d6
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