Skip to content

Conversation

@richafrank
Copy link
Member

A few unrelated improvements

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just broken in the current incarnation? I thought we had tests for this case...

Copy link

Choose a reason for hiding this comment

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

I see tests for renaming directories, but I couldn't find tests on how checkpoints interact with things:
https://github.com/quantopian/pgcontents/blob/master/pgcontents/tests/test_pgmanager.py#L124

I'm not quite sure what the test logic would be either & tried to find what actually calls rename_checkpoint and couldn't figure out where it would be:
https://github.com/jupyter/notebook/search?utf8=%E2%9C%93&q=rename_checkpoint

EDIT: Nevermind, I just fail at searching, rename_all_checkpoints calls it and is called by rename: https://github.com/jupyter/notebook/blob/a6c5be5d60f122400b65d9c8d6488e78f25b9c2f/notebook/services/contents/manager.py#L231

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

rename_checkpoint is only called by rename_all_checkpoints in the base class, but PostgresCheckpoints overrides rename_all_checkpoints to rename all the checkpoints in a single SQL statement, instead of renaming them one by one. I'm going to replace this fix with raise NotImplementedError() to show that it's not currently expected that something would call it.

It's not used by PostgresCheckpoints.rename_all_checkpoints,
which can rename all checkpoints at once.
richafrank added a commit that referenced this pull request Jul 16, 2015
@richafrank richafrank merged commit a4243c0 into master Jul 16, 2015
@richafrank richafrank deleted the migration_fixes branch July 16, 2015 17:58
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.

4 participants