jacebrowning / datafiles

A file-based ORM for Python dataclasses.
https://datafiles.readthedocs.io
MIT License
198 stars 18 forks source link

Support renaming files if the filename attributes were modified #304

Open tbabej opened 1 year ago

tbabej commented 1 year ago

This MR implements the necessary logic to allow the Mapper to write the datafile to a new location if the filepath has changed due to the user changing the attributes that are reflected in the filepath itself. The behaviour is gated behind rename=True flag (or its equivalents in the Meta class).

Prior to merging:

Closes #303.

tbabej commented 1 year ago

@jacebrowning if you think this is looking OK, I will add tests and docs. IMHO the changes for filename attributes to be immutable without rename=True should be implemented in a separate PR, and I am happy to take that on next.

jacebrowning commented 1 year ago

@tbabej since I don't see anywhere an actual rename is performed, does that suggest that this feature already (implicitly) exists, aside from leaving the old filename around?

If that's the case then perhaps a simpler "beta gate" would be add a new RENAMES_ENABLED setting to provide this functionality:

What do you think?

tbabej commented 1 year ago

@tbabej since I don't see anywhere an actual rename is performed, does that suggest that this feature already (implicitly) exists, aside from leaving the old filename around?

My apologies for not making this more clear in the commit messages, this MR actually currently fully implements the feature. The logic is as follows:

  1. The main reason why renaming (or rather, writing out file to a new location based on new "filename" attributes) was not supported in the first place is because self.path is a cached property. This means that the filepath stays the same since it was first evaluated no matter what modifications were preformed to the filename attributes.
  2. The file modifications to mapper.py result in a file rename in a two step process. First, this section https://github.com/jacebrowning/datafiles/blob/c47ca620fbcdc00897387763e794572e4af00501/datafiles/mapper.py#L272-L282

detects whether the file location has changed by invalidating the cache of the self.path cached property on line 278 and forcing its recomputation in the if statement on line 281. In case it is changed, the file_rename_required flag is set and subsequently https://github.com/jacebrowning/datafiles/blob/c47ca620fbcdc00897387763e794572e4af00501/datafiles/mapper.py#L298-L300

on the line 298 the file content is written out to the new location. The old location is subsequently removed if the path was changed. So technically this is a "write to a new filepath" and "remove old filepath"

If that's the case then perhaps a simpler "beta gate" would be add a new RENAMES_ENABLED setting to provide this functionality:

  • True = deleted the old filename when a rename is detected
  • False (initial default) = warn that a filename attribute was changed

What do you think?

I'm open to either solution. Ultimately in my opinion this comes down to whether we think it's going to be useful to selectively enable renames in certain datafiles, but not in others. I guess I'm leaning towards yes (i.e. something like Jane-Doe.yml makes sense to be renamed Jane-Smith.yml if the person's name is changed, but if my filename is called 0702dce3-764b-4bcf-83e3-a7ae417555b1.yml, i.e. some immutable UUID, it would make more sense for the "frozen attribute" behaviour to occur. And it's feasible for both situations to happen within the same application.

jacebrowning commented 1 year ago

@tbabej thanks for the thorough explanation!

this comes down to whether we think it's going to be useful to selectively enable renames in certain datafiles, but not in others. I guess I'm leaning towards yes

I'm inclined to agree. Please proceed with adding tests and documentation!

devlounge commented 1 month ago

Any update on this? This was a nice to have feature, sad it is stale since

tbabej commented 1 month ago

@devlounge would you be willing to lend a hand? I can rebase this MR on top of the newest main, but would need help with adding documentation/tests