Issue #50 resolved

More descriptive variable names

created an issue

I recently forked alembic, and have been making a lot of changes to implement multi-schema support. One thing I've noticed is the variable 'tname' tends to be used frequently to refer to what sqlalchemy considers Table.fullname (schema.tablename if schema is present, else tablename). tname implies (to me, at least), which has no schema. Currently, we have the variable tname frequently referring to the table's fullname, yet being passed into functions which will break if they receive a fullname (backticks around a dotted tablename, ie schema.table, rather than schema.table)

It might be a good idea to start distinguishing the two, perhaps via a tfullname vs tname naming scheme based on which format you're referring to, that way when you see a random reference to a tablename in a file, you'll know whether or not it needs special handling to coerce to something usable.

Also, given that so many functions takes a 'schema' parameter, it might be a good idea to take special care when using 'from sqlalchemy import schema', as that then requires aliasing of the schema kwarg in places, which makes things inconsistent. Again, a minor issue, but my fork is now generating migration files where some commands have a "schema" kwarg, and some have "schema_". Given the destination endpoint for all the commands is in the impl file, and those are all set up to accept the 'schema' kwarg, it would be nice to be able to pass it through without concerning oneself with aliasing it partway through the process.

Comments (2)

  1. Mike Bayer repo owner

    I did a search for the name "tname", this is where I found it:

    1. it appears in one private function in, as an internal-only variable, where it refers to

    2. it appears as an argument in the private function>_exec_drop_col_constraint, and is one of the positional arguments. it refers only to

    3. it is used in within several private functions. In all cases, it is currently assumed to be a table name only, even though it is referred to as a key within the metadata.tables argument, where if the table were schema-qualified, the name would include a schema. However, does not support "schema" in any way right now - you'll note that tname is fed directly into the Table construct as the "name" in many areas. As #33 is addressed, lots of this code will have to change significantly in any case, so certainly within autogenerate we can call it "tfullname" when referring to a key in metadata.tables (and if you're in fact attempting to implement #33 for us, then yes go ahead and do that).

    In all of the above cases, "tname" is only referred to positionally. If you're working on #33, feel free to change it. Also there's no place in the current Alembic source that the word "fullname" is referred to at all, so that's on your end.

    For #33, the name of the kw argument will be "schema", to be consistent with the same argument name used in Table, Index, Sequence. It's normal practice that we have coverage for all codelines so I'm not very concerned about "schema" as a kw arg being confused for those places that "schema" as a module is used. If you'd like to submit a pull request which replaces usages of the "schema" module, i.e. "schema.SomeClass", with imports for each specific DDLElement we are calling from it, feel free.

    I'm not getting the sense that this ticket is really separate from the work within #33 ? Is there something specific here to do that isn't better included on that ticket ? I'm trying to keep the number of tickets low.

  2. dieselmachine reporter

    Alembic doesn't use 'fullname' anywhere, but the fullname is what is being used as the key for metadata_tables (in autogenerate), so as long as we're going to be doing comparisons based on it, while being unable to pass that name to the next steps in the chain, we should disambiguate them so the code is clear with regards to which 'version' is being used.

    The ticket really isn't separate from #33, sorry for littering. :D It was half suggestion, half 'testing the waters' to see what your view on the issue was, because I'd like to start submitting pull requests, but I really want to overhaul autogenerate and rename a lot of things (all in private functions), and I wasn't sure how you'd feel about that, so I figured I'd explain where I was coming from.

    It seems you're okay with me using more specific variable names in autogenerate, and replacing the schema import with more specific imports, so I'll work on getting those implemented along with some support for schema-related things. I'm new to mercurial so I need to learn a few things first.

    My updated autogenerate file has support for multi-schemas, and an interactive mode that guides the user with regards to handling renamed tables and columns. Once they are more fully tested I'll be pushing some changes back.

  3. Log in to comment