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

Implement #123 arbitrary Mongo URL & collection name #124

Merged
merged 3 commits into from Mar 12, 2020

Conversation

EliseAv
Copy link
Contributor

@EliseAv EliseAv commented Mar 6, 2020

I have also updated the documentation a bit.

The purpose of the function AsyncIOMotorClient.get_default_database is to allow for the Mongo URL to define a database. If the URL contains a database, that database in the URL will be used, otherwise, the calculated database name is used. As for get_collection, I changed it for consistency only.

I wanted to update the unit tests, but they were skipped… and when I tried to un-skip them everything failed spetacularly. I'm not sure what to do about that.

Cheers!

@ekzhu
Copy link
Owner

ekzhu commented Mar 7, 2020

Thanks for the pull request. @aastafiev I remember the unit test for Mongo was skipped because of Python version incompatibility?

@aastafiev
Copy link
Contributor

aastafiev commented Mar 7, 2020 via email

@EliseAv
Copy link
Contributor Author

EliseAv commented Mar 7, 2020 via email

@ekzhu
Copy link
Owner

ekzhu commented Mar 8, 2020

@ekevoo So is your unit tests failure caused by Python version issue?

@EliseAv
Copy link
Contributor Author

EliseAv commented Mar 10, 2020

Turns out the check was already there:

@unittest.skipIf(sys.version_info < (3, 6), "Skipping TestAsyncMinHashLSH. Supported Python version >= 3.6")

Travis had accepted the changes. I initially didn't try very hard to run unit tests, it turns out that they do require a running MongoDB server to run, which is a little problematic, but I'm not qualified to fix. So I just ran a local server (docker run -p 27017:27017 mongo) and created new tests to test my changes. Everything passed for me, I hope Travis agrees.

@ekzhu ekzhu merged commit e5f14ae into ekzhu:master Mar 12, 2020
@ekzhu
Copy link
Owner

ekzhu commented Mar 12, 2020

Thanks @ekevoo!

@EliseAv
Copy link
Contributor Author

EliseAv commented Mar 17, 2020

Thank you too! @ekzhu and @aastafiev! :) Can you tell when you might next release it?

@ekzhu
Copy link
Owner

ekzhu commented Mar 18, 2020

I already have. https://pypi.org/project/datasketch/1.5.1/#history 🚢

@EliseAv
Copy link
Contributor Author

EliseAv commented Mar 18, 2020

Ah, I was checking the releases tab instead. 😅
Thank you very much ✌️

@EliseAv
Copy link
Contributor Author

EliseAv commented Mar 19, 2020

Hi, the version that was published on PIP as 1.5.1 doesn't contain this.

@ekzhu
Copy link
Owner

ekzhu commented Mar 19, 2020

Are you sure? I just checked this source distribution and I can see the new code in there.

@EliseAv
Copy link
Contributor Author

EliseAv commented Mar 22, 2020 via email

@ekzhu
Copy link
Owner

ekzhu commented Mar 22, 2020

Thanks. Could you show a simple script that demonstrate it? I don't have Mongo Atlas.

@EliseAv
Copy link
Contributor Author

EliseAv commented Mar 23, 2020

That's okay, you can test using local mongo with the new URL feature.

storage_config={"type": "aiomongo",
                "mongo": {"url": "mongodb://localhost/databaseName"}}

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.

None yet

3 participants