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

Automatically parse the dbname from the URI #350

Merged
merged 6 commits into from
Dec 2, 2020
Merged

Automatically parse the dbname from the URI #350

merged 6 commits into from
Dec 2, 2020

Conversation

jmmshn
Copy link
Contributor

@jmmshn jmmshn commented Dec 2, 2020

  • Read the database name from the URI, still allow the user-set value to supercede this.

@jmmshn
Copy link
Contributor Author

jmmshn commented Dec 2, 2020

@shyamd I need to access secrets to add a test for this.

@codecov
Copy link

codecov bot commented Dec 2, 2020

Codecov Report

Merging #350 (1d3e191) into master (b3ec833) will decrease coverage by 0.19%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #350      +/-   ##
==========================================
- Coverage   87.55%   87.35%   -0.20%     
==========================================
  Files          28       28              
  Lines        2186     2191       +5     
==========================================
  Hits         1914     1914              
- Misses        272      277       +5     
Impacted Files Coverage Δ
src/maggma/stores/mongolike.py 81.71% <100.00%> (-1.63%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d488060...1d3e191. Read the comment docs.

@shyamd
Copy link
Contributor

shyamd commented Dec 2, 2020

You should be able to use a dummy URI to test if the parsing is working.

@jmmshn
Copy link
Contributor Author

jmmshn commented Dec 2, 2020

OK awesome! I thought MongoClient needed to actually connect or something. Guess not

Comment on lines 379 to 389
if database is None:
with MongoClient(self.uri) as conn:
try:
db = conn.get_default_database()
except ConfigurationError:
raise ConfigurationError(
"If database name is not supplied, a database must be set in the uri"
)
self.database = db.name
else:
self.database = database
Copy link
Contributor

Choose a reason for hiding this comment

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

does self.database even need to be stored if it's not provided; IE, does anything break if database = None? Also, this should probably be in connect ? Connection on init can be bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok using the Mongoclient there was a little hacky.
I have changed it.
Technically the code will not break if database == None but it will break if database == None and no value is found in the URI.
I do think the database should be kept since every Mongo-like store must have a database anyways.

@shyamd shyamd merged commit 12266fd into materialsproject:master Dec 2, 2020
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