Skip to content

Conversation

juliusgeo
Copy link
Contributor

No description provided.

@juliusgeo juliusgeo linked an issue Aug 3, 2020 that may be closed by this pull request
@juliusgeo juliusgeo changed the title Add aliases to resolve discrepancy in namine Add aliases to resolve discrepancy in naming Aug 3, 2020
Copy link
Collaborator

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

We should tests that:

  • Both ExplainCollection and ExplainableCollection can be imported directly from pymongoexplain.
  • ExplainCollection and ExplainableCollection are the same.

@juliusgeo juliusgeo requested a review from ShaneHarvey August 3, 2020 20:02
# Alias
ExplainCollection = ExplainableCollection


Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there are some extra newlines here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

except:
print("Could not import!")
assert False
assert ExplainableCollection == ExplainCollection
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove the try/excepts and use self.assertEqual instead of bare assert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

try:
from pymongoexplain import ExplainCollection
except:
print("Could not import!")

Choose a reason for hiding this comment

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

You can replace these two lines with:

self.fail("Failed to import ExplainCollection from pymongoexplain")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

from pymongoexplain import ExplainableCollection
except:
print("Could not import!")
assert False

Choose a reason for hiding this comment

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

Same comment as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

except:
print("Could not import!")
assert False
try:

Choose a reason for hiding this comment

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

Suggest leaving a newline between the two sub-tests to make this more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -213,5 +213,11 @@ def test_cli_tool(self):
self.assertNotEqual(res.stdout, "")
self.assertTrue(res.returncode == 0)

def test_imports(self):
from pymongoexplain import ExplainCollection

Choose a reason for hiding this comment

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

Nice, simple solution!

Copy link

@prashantmital prashantmital left a comment

Choose a reason for hiding this comment

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

I think after #33 is merged, you need to rebase this on top of master and add a changelog entry for the addition of the alias.

@juliusgeo
Copy link
Contributor Author

I think after #33 is merged, you need to rebase this on top of master and add a changelog entry for the addition of the alias.

Done.

@juliusgeo juliusgeo merged commit 92e7264 into mongodb-labs:master Aug 5, 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.

Discrepancy between submodule name and ExplainCollection classname
3 participants