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

Fix alembic.ini templates to match configparser file format. #1397

Closed
wants to merge 4 commits into from

Conversation

bhushan-mohanraj
Copy link

@bhushan-mohanraj bhushan-mohanraj commented Jan 13, 2024

In the alembic.ini templates, I moved the inline comment about version_path_separator to their own lines as required by configparser.

Description

In a recent project, I included the following configuration values in my alembic.ini. Note that the last line is the default line from the current generic alembic.ini template.

# version location specification; This defaults
# to migrations/versions.  When using multiple version
# directories, initial revisions must be specified with --version-path.
# The path separator used here should be the separator specified by "version_path_separator" below.
version_locations = %(here)s/migrations/versions

# version path separator; As mentioned above, this is the character used to split
# version_locations. The default within new alembic.ini files is "os", which uses os.pathsep.
# If this key is omitted entirely, it falls back to the legacy behavior of splitting on spaces and/or commas.
# Valid values for version_path_separator are:
#
# version_path_separator = :
# version_path_separator = ;
# version_path_separator = space
version_path_separator = os  # Use os.pathsep. Default configuration used for new projects.

When running alembic check, I encountered:

ValueError: 'os  # Use os.pathsep. Default configuration used for new projects.' is not a valid value for version_path_separator; expected 'space', 'os', ':', ';'

It seemed that the comment in the last line was being included as part of the parsed config value, which should be os.

Alembic currently uses configparser.ConfigParser from the standard libary to parse alembic.ini files. The default configparser file format requires that comments be on their own lines, although this can be customized. I changed the three copies of this line in Alembic's alembic.ini templates to remove the inline comments. In my case, this change fixed the ValueError.

This issue could also be fixed by changing the default instance of ConfigParser, using inline_comment_prefixes=("#",). I imagine, however, that it might be better to use the default file format.

Checklist

This pull request is:

  • A documentation / typographical error fix
    • Good to go, no issue or tests are needed
  • A short code fix
    • please include the issue number, and create an issue if none exists, which
      must include a complete example of the issue. one line code fixes without an
      issue and demonstration will not be accepted.
    • Please include: Fixes: #<issue number> in the commit message
    • please include tests. one line code fixes without tests will not be accepted.
  • A new feature implementation
    • please include the issue number, and create an issue if none exists, which must
      include a complete example of how the feature would look.
    • Please include: Fixes: #<issue number> in the commit message
    • please include tests.

Have a nice day!

@CaselIT CaselIT requested a review from zzzeek January 13, 2024 21:58
@CaselIT
Copy link
Member

CaselIT commented Jan 13, 2024

Thanks

Seems to be a reasonable change

@zzzeek zzzeek requested a review from sqla-tester November 6, 2024 20:47
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision f7198e3 of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Failed to create a gerrit review, git squash against branch 'main' failed

@zzzeek
Copy link
Member

zzzeek commented Nov 6, 2024

I need this PR to be rebased on main for me to work with it.

@zzzeek zzzeek requested a review from sqla-tester November 6, 2024 23:43
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 073cbd9 of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

New Gerrit review created for change 073cbd9: https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5587

@zzzeek
Copy link
Member

zzzeek commented Nov 6, 2024

OK I used github's UX for this

@sqla-tester
Copy link
Collaborator

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/5587 has been merged. Congratulations! :)

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.

4 participants