Skip to content

Comments

Default SQLALCHEMY_DATABASE_URI to None, error if no URI or binds#731

Merged
davidism merged 1 commit intopallets-eco:masterfrom
lbeaufort:663-add-error-missing-uri-no-binds
Oct 11, 2019
Merged

Default SQLALCHEMY_DATABASE_URI to None, error if no URI or binds#731
davidism merged 1 commit intopallets-eco:masterfrom
lbeaufort:663-add-error-missing-uri-no-binds

Conversation

@lbeaufort
Copy link
Contributor

@lbeaufort lbeaufort commented May 8, 2019 •
edited
Loading

Resolves #663

@lbeaufort lbeaufort force-pushed the 663-add-error-missing-uri-no-binds branch 2 times, most recently from 733d02f to 959c419 Compare May 8, 2019 20:41
@lbeaufort lbeaufort requested a review from rsyring May 8, 2019 20:43
@lbeaufort lbeaufort changed the title [WIP] Default SQLALCHEMY_DATABASE_URI to None, error if no URI or binds Default SQLALCHEMY_DATABASE_URI to None, error if no URI or binds May 8, 2019
@lbeaufort lbeaufort requested a review from davidism May 8, 2019 20:53
@lbeaufort lbeaufort mentioned this pull request May 12, 2019
Closed
davidism
davidism reviewed May 13, 2019
View reviewed changes
Copy link
Member

@davidism davidism left a comment

Choose a reason for hiding this comment

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

Minor formatting change, otherwise looks good 👍

docs/config.rst Outdated

.. versionchanged:: 3.0

* ``SQLALCHEMY_TRACK_MODIFICATIONS`` configuration key now defaults to ``False``
Copy link
Member

@davidism davidism May 13, 2019

Choose a reason for hiding this comment

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

These need to be indented by 4 spaces, same with the previous block.

Copy link
Contributor Author

@lbeaufort lbeaufort Oct 11, 2019

Choose a reason for hiding this comment

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

Hi @davidism sorry this slipped my radar, I will push up the change now.

Copy link
Contributor Author

@lbeaufort lbeaufort Oct 11, 2019

Choose a reason for hiding this comment

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

Just updated - it looks like the block above was 2 spaces indented, please let me know if I got that wrong.

@lbeaufort lbeaufort force-pushed the 663-add-error-missing-uri-no-binds branch 2 times, most recently from 9ff7bef to e1c4bc8 Compare October 11, 2019 13:50
@lbeaufort
Copy link
Contributor Author

lbeaufort commented Oct 11, 2019

@davidism I just fixed a merge conflict, so I think this is ready for your review again. There's one failing travis test but it looks like it might be a quirk if you don't mind retrying.

Please let me know if you find any issues. Apologies for the delay!

@davidism davidism removed the request for review from rsyring October 11, 2019 14:26
@davidism
Copy link
Member

davidism commented Oct 11, 2019

I have no idea what's causing py36-lowest to fail, but I can reproduce it locally.

@lbeaufort
Copy link
Contributor Author

lbeaufort commented Oct 11, 2019

Ok thanks! I will take a look too.

@lbeaufort
Copy link
Contributor Author

lbeaufort commented Oct 11, 2019

It looks like Travis has been failing tests on master for a few commits. I'm going to try to set up the tests locally to see what I can figure out.

@lbeaufort
Copy link
Contributor Author

lbeaufort commented Oct 11, 2019 •
edited
Loading

Looks like maybe the issue is that these tests are bringing in Flask==0.12 which doesn't include this bug fix? pallets/flask#3278

Relevant issues: pytest-dev/pytest#5532 and pallets/flask#3275

py36-lowest installed: ...Flask==0.12...pytest==5.2.1...

E   AttributeError: AssertionRewritingHook.is_package() method is missing but is required by Flask of PEP 302 import hooks.  If you do not use import hooks and you encounter this error please file a bug against Flask.

@davidism
Copy link
Member

davidism commented Oct 11, 2019

Nice, using Flask 1.0.4 works. See #787 for bumping dependencies.

@davidism davidism mentioned this pull request Oct 11, 2019
Closed
@davidism
Copy link
Member

davidism commented Oct 11, 2019

OK, that's merged in to master now, you can rebase and test this again.

@lbeaufort lbeaufort force-pushed the 663-add-error-missing-uri-no-binds branch from e1c4bc8 to 06e0e12 Compare October 11, 2019 16:14
@lbeaufort
Default SQLALCHEMY_DATABASE_URI to None, error if no URI or binds
bde166e 
- Default SQLALCHEMY_DATABASE_URI to None
- Throw an error when SQLALCHEMY_DATABASE_URI and SQLALCHEMY_BINDS are unset
- Update tests
- Update docs
- Update changelog, fix formatting for previous change
@lbeaufort lbeaufort force-pushed the 663-add-error-missing-uri-no-binds branch from 06e0e12 to bde166e Compare October 11, 2019 16:15
@lbeaufort
Copy link
Contributor Author

lbeaufort commented Oct 11, 2019

@davidism awesome, thanks! Rebased and resolved conflicts and tests now pass.

@davidism davidism merged commit db02fd4 into pallets-eco:master Oct 11, 2019
@davidism
Copy link
Member

davidism commented Oct 11, 2019

Cool, we're good to go. Thanks for working on this!

@lbeaufort lbeaufort deleted the 663-add-error-missing-uri-no-binds branch October 11, 2019 16:26
@zhongxinghong zhongxinghong mentioned this pull request Feb 24, 2020
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@davidism davidism davidism approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Disable SQLALCHEMY_DATABASE_URI when SQLALCHEMY_BINDS is used

2 participants

@lbeaufort @davidism