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

Add colon (:) to invalid char for revision name #1541

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PatilHrushikesh
Copy link

@PatilHrushikesh PatilHrushikesh commented Sep 22, 2024

FIXES #1540

Description

As colon is used in revision range, it shouldn't be used in revision name.

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!

As colon is used in revision range, it shouldn't be used in revision name
@zzzeek
Copy link
Member

zzzeek commented Sep 22, 2024

it's too bad because the colon isn't any problem for the revision API, it's just used by the command interface. colon could be supported if the command interface had a function that parsed for quoted revision names. but then we'd probably want quotes added to the illegal chars also

@PatilHrushikesh
Copy link
Author

Can you explain

it's just used by the command interface.

Yeah, and it's causing issue mentioned in ticket, is there better way to fix it?

quoted revision

How does quoted revision comes into the picture?

@zzzeek
Copy link
Member

zzzeek commented Sep 23, 2024

the colon is only parsed inside of command.py, it's not used in revision.py

we could also say that we would support this syntax:

alembic upgrade --sql "some:rev":ae5c

however, there's no compelling reason for that ...

@CaselIT
Copy link
Member

CaselIT commented Sep 23, 2024

I think the solution proposed in this PR is likely easier to implement. What do you think mike?

@zzzeek
Copy link
Member

zzzeek commented Sep 23, 2024

it is. can you add a changelog and do we have illegal char tests set up? we should

@CaselIT
Copy link
Member

CaselIT commented Sep 23, 2024

@PatilHrushikesh

  • could you add an additional case (or edit an existing one) here
    def test_illegal_revision_chars(self):
    assert_raises_message(
    util.CommandError,
    r"Character\(s\) '-' not allowed in "
    "revision identifier 'no-dashes'",
    command.revision,
    self.cfg,
    message="some message",
    rev_id="no-dashes",
    )
    assert not os.path.exists(
    os.path.join(self.env.dir, "versions", "no-dashes_some_message.py")
    )
    assert_raises_message(
    util.CommandError,
    r"Character\(s\) '@' not allowed in "
    "revision identifier 'no@atsigns'",
    command.revision,
    self.cfg,
    message="some message",
    rev_id="no@atsigns",
    )
    assert_raises_message(
    util.CommandError,
    r"Character\(s\) '-, @' not allowed in revision "
    "identifier 'no@atsigns-ordashes'",
    command.revision,
    self.cfg,
    message="some message",
    rev_id="no@atsigns-ordashes",
    )
    assert_raises_message(
    util.CommandError,
    r"Character\(s\) '\+' not allowed in revision "
    r"identifier 'no\+plussignseither'",
    command.revision,
    self.cfg,
    message="some message",
    rev_id="no+plussignseither",
    )
    to have a :?
  • could you add a changelog entry here? examples are here https://github.com/sqlalchemy/alembic/tree/3db374f19760320507a96443e29d19835654b2ec/docs/build/unreleased the number is the issue id

Thanks!

@zzzeek
Copy link
Member

zzzeek commented Nov 6, 2024

is this PR stalled ? it needs the additional change as well as a unit test

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.

Colon (:) shouldn't be allowed in custom revision id, as it is used as revision range
3 participants