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 constraint naming convention configuration option #197

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

leonarduschen
Copy link
Contributor

Closes #193

@agronholm
Copy link
Owner

FYI, I'm planning one last big refactoring before the final release (one which should resolve the issue of code duplication) and it will probably clash with this PR.

@leonarduschen
Copy link
Contributor Author

Right, I'm actually working on 2 additional issues, details outlined in #193 (comment)

Those issues are only relevant when using these 2 types of tokens:

  • %(referred_column_0_name)s, %(referred_column_0_N_name)s, %(referred_column_0N_name)s, etc
  • %(constraint_name)s

I was thinking of doing everything in one go in this PR, but if you're going to do a big refactoring then perhaps I should just fix those in subsequent PRs? For now we can add a note in the docs that those tokens cannot be used.

Let me know what you think.

@coveralls
Copy link

coveralls commented Apr 26, 2022

Coverage Status

Coverage increased (+0.02%) to 99.744% when pulling f501697 on leonarduschen:add-constraint-naming into 5029eb1 on agronholm:master.

@agronholm
Copy link
Owner

I've opened #200 to address the design choice of config file support which we probably need for this.

"--conv",
nargs="*",
help='constraint naming conventions in "key=template" format \
e.g., --conv "pk=pk_%%(table_name)s" "uq=uq_%%(table_name)s_%%(column_0_name)s"',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the option to add the naming convention via command line on top of config file (which hasn't been added yet)

I was thinking, for each config option, we can go like this:

  • Check if it is available as command line arg
  • If not, check if it is available in config file
  • If not, fall back to default value

Copy link
Owner

Choose a reason for hiding this comment

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

How would users know what to enter here, based on this?

Copy link
Contributor Author

@leonarduschen leonarduschen Apr 28, 2022

Choose a reason for hiding this comment

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

Well we assume users interested in using this option are already familiar with SQLAlchemy's constraint naming convention feature from https://docs.sqlalchemy.org/en/14/core/constraints.html#configuring-constraint-naming-conventions or https://alembic.sqlalchemy.org/en/latest/naming.html. In this case, the help string above would enough in my opinion.

We can also add more details in README:

  • Link to SQLAlchemy/Alembic docs above
  • Maybe also basic info about constraint naming convention and its usage in SQLAlchemy, basically summarizing the docs above
  • Things supported in SQLAlchemy but not in sqlacodegen:
    • Using custom function as naming convention token
    • Using the constraint object name as key instead of mnemonic (e.g., PrimaryKeyConstraint cannot be used as key, must use pk)

@leonarduschen leonarduschen marked this pull request as ready for review April 27, 2022 13:02
@leonarduschen
Copy link
Contributor Author

I've addressed the 2 issues I mentioned earlier #193 (comment) - ready for review now

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.

Add constraint naming convention configuration option
3 participants