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

Rename package to sqlalchemy_redshift #58

Merged
merged 2 commits into from
Oct 24, 2015
Merged

Rename package to sqlalchemy_redshift #58

merged 2 commits into from
Oct 24, 2015

Conversation

jklukas
Copy link
Member

@jklukas jklukas commented Oct 23, 2015

Fixes #40.

)

# All references to module redshift_sqlalchemy will map to sqlalchemy_redshift
sys.modules['redshift_sqlalchemy'] = sqlalchemy_redshift
Copy link
Member Author

Choose a reason for hiding this comment

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

I just discovered sys.modules. This appears to do the magic that we want. It's possible to do imports like from redshift_sqlalchemy.dialect import RedshiftDDLCompiler.

Copy link
Member

Choose a reason for hiding this comment

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

this is correct but according to the six.py source code this method breaks:

from redshift_sqlalchemy import dialect
reload(dialect)

but I don't mind.

Can you add a deprecation warning to this module?

@graingert
Copy link
Member

It would be quite nice if you could you split this into two commits and submit them one at a time so travis sees each one:

  1. where you
    • rename the module redshift_sqlalchemy
    • create the compat module
    • update the changelog
  2. where you update the tests + docs to point at sqlalchemy_redshift

This way we can prove the compatability module works

But I'm not fussed if you don't want to, I'm still happy to merge this as is

@jklukas jklukas force-pushed the rename-package branch 3 times, most recently from dd9d4f0 to b99f027 Compare October 23, 2015 14:28
a future release, so it is recommended to update all package references.
"""

warnings.warn(DEPRECATION_MESSAGE, DeprecationWarning)
Copy link
Member Author

Choose a reason for hiding this comment

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

DeprecationWarning is ignored by default, but would seem to be the semantically correct choice here.

Copy link
Member

Choose a reason for hiding this comment

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

nice

@jklukas
Copy link
Member Author

jklukas commented Oct 23, 2015

I understand the philosophy of why you'd want to split this up, but I'm not inclined to put in the extra work to make it happen.

@jklukas
Copy link
Member Author

jklukas commented Oct 23, 2015

Rebased to edit the commit with broken README.rst.

@@ -1,7 +1,10 @@
0.3.2 (unreleased)
------------------

- Nothing changed yet.
- Change the name of the package to `sqlalchemy_redshift` to match the naming
convention for other dialects; the `redshift_sqlalchemy` now references
Copy link
Member

Choose a reason for hiding this comment

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

missed a word? Should this be "the deprecated redshift_sqlalchemy now references..."?

@jklukas
Copy link
Member Author

jklukas commented Oct 23, 2015

Updated the CHANGELOG message.

@graingert
Copy link
Member

Perfect! Merge when ready

jklukas added a commit that referenced this pull request Oct 24, 2015
Rename package to sqlalchemy_redshift
@jklukas jklukas merged commit a85c4c1 into master Oct 24, 2015
@jklukas jklukas deleted the rename-package branch May 4, 2017 18:52
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