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

Support renaming files if the filename attributes were modified #304

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

tbabej
Copy link
Contributor

@tbabej tbabej commented Jun 13, 2023

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:

  • tests to cover the behavior should be added
  • documentation should be added

Closes #303.

@tbabej tbabej force-pushed the main branch 2 times, most recently from 5abacc2 to 95ea5c4 Compare June 14, 2023 01:20
@tbabej
Copy link
Contributor Author

tbabej commented Jun 14, 2023

@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.

@tbabej tbabej changed the title WIP: Support renaming files if the filename attributes were modified Support renaming files if the filename attributes were modified Jun 14, 2023
@jacebrowning
Copy link
Owner

@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:

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

What do you think?

@tbabej
Copy link
Contributor Author

tbabej commented Jun 14, 2023

@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
    file_rename_required = False
    original_path = self.path
    if self._rename:
    with hooks.disabled(): # hooks have to be disabled to prevent infinite loop
    if "path" in self.__dict__:
    del self.__dict__["path"] # invalidate the cached property
    # This call of self.path updates the value since the cache is invalidated
    if self.path != original_path:
    file_rename_required = True

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

write(self.path, text, display=True)
if self._rename and file_rename_required:
remove(original_path)

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
Copy link
Owner

@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!

@jacebrowning jacebrowning marked this pull request as draft January 20, 2024 21:14
@devlounge
Copy link

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

@tbabej
Copy link
Contributor Author

tbabej commented Jul 18, 2024

@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

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.

Allow synchronization of filename attributes
3 participants