PYTHON-4946 - Add GridFSBucket.rename_by_name#2219
Conversation
…lename' feature - rename_by_name
gridfs/asynchronous/grid_file.py
Outdated
| ) | ||
| if not result.matched_count: | ||
| raise NoFile( | ||
| f"no files could be renamed {new_filename} because none matched filename {filename}" |
There was a problem hiding this comment.
Suggest: f"no files could be renamed {new_filename!r} because none matched filename {filename!r}" to make the strings more obvious.
| "matched file_id %i" % (new_filename, file_id) | ||
| ) | ||
|
|
||
| async def rename_by_name( |
There was a problem hiding this comment.
Instead of adding a new method for this, should we consider overloading the existing rename method? For example:
>>> fs.rename(file_id, "new") # rename by id
>>> fs.rename(filename="old", new_filename="new") # rename by nameThe pro is we have less apis to maintain. I guess a con is that the user could accidentally mix up the file_id and filename params.
Note I'm not advocating for either, I just think we should consider it and maybe ask the spec author about it.
There was a problem hiding this comment.
I like this suggestion--both methods have nearly identical bodies, so combining them makes sense. For #2218, the two methods have slightly different bodies, but still similar enough that we could do the same thing there. Thoughts?
There was a problem hiding this comment.
What would the method signatures looks like (including type hints)?
There was a problem hiding this comment.
After trying this out a bit, I think it's a little too confusing/unclear to combine them.
There was a problem hiding this comment.
Could you share the signatures for posterity?
There was a problem hiding this comment.
async def rename(file_id: Optional[Any], new_filename: str, filename: Optional[str] = None,...)
gridfs/asynchronous/grid_file.py
Outdated
| :param filename: The filename of the file to be renamed. | ||
| :param new_filename: The new name of the file. | ||
| :param session: a | ||
| :class:`~pymongo.client_session.AsyncClientSession` |
There was a problem hiding this comment.
our line length allows this to be one line now right?
:param session: a :class:`~pymongo.client_session.AsyncClientSession`
gridfs/asynchronous/grid_file.py
Outdated
| await fs.upload_from_stream("test_file", "data I want to store!") | ||
| await fs.rename_by_name("test_file", "new_test_name") | ||
|
|
||
| Raises :exc:`~gridfs.errors.NoFile` if no file with file_id exists. |
There was a problem hiding this comment.
with file_id -> with the given filename
test/asynchronous/unified_format.py
Outdated
| elif isinstance(error, (InvalidOperation, ConfigurationError, EncryptionError)): | ||
| pass | ||
| # gridfs NoFile errors are considered client errors. | ||
| elif isinstance(error, NoFile): |
There was a problem hiding this comment.
Suggest adding this to the previous elif: InvalidOperation, ConfigurationError, EncryptionError, NoFile)):
| "matched file_id %i" % (new_filename, file_id) | ||
| ) | ||
|
|
||
| def rename_by_name( |
There was a problem hiding this comment.
Let's add some hand written tests for this too like test_rename in test_gridfs_bucket. That way we get some signal on typing and the UX.
ShaneHarvey
left a comment
ShaneHarvey
left a comment
There was a problem hiding this comment.
One minor comment. Otherwise LGTM.
doc/changelog.rst
Outdated
| :class:`~pymongo.encryption_options.AutoEncryptionOpts`. | ||
| - Support for $lookup in CSFLE and QE supported on MongoDB 8.1+. | ||
| - Added :meth:`gridfs.asynchronous.grid_file.AsyncGridFSBucket.rename_by_name` and :meth:`gridfs.grid_file.GridFSBucket.rename_by_name` | ||
| for more performant renaming of file revisions. |
There was a problem hiding this comment.
of file revisions. -> of a file with multiple revisions.
ShaneHarvey
changed the title
ShaneHarvey
left a comment
ShaneHarvey
left a comment
There was a problem hiding this comment.
LGTM, test failures are unrelated.
2 participants
…lename' feature - rename_by_name