feat(rename): allow batch and auto-renaming from pattern #810

kiike · 2024-04-10T09:23:14Z

When I develop a feature, I try to dogfood, so sometimes I end up using default values of config options such as add-folder-name, thus creating folders that don't end up like I like. papis rename can rename entries but is now pretty limited.

With this WIP pull request, I am trying to make papis rename a bit more powerful:

allow selecting multiple entries with the picker
selecting all entries matching a query, for mass renaming of folders
optional slugify of names
rename without confirmation
rename using the generated folder name

It's taking form:

With slug enabled:

 papis rename -arsb "black hole"
[INFO] commands.rename: Renaming 'raju-2024-howdoesinformationemerge' into 'test-2024-how-does-information-emerge-from-a-black-hole'
[INFO] commands.rename: Renaming 'macedo-2024-thermodynamicsandquasinormalmodes' into 'test-2024-thermodynamics-and-quasinormal-modes-of-the-dymnikova-black-hole-in-higher-dimensions'
[INFO] commands.rename: Renaming 'ressler-2024-blackhole-diskinteractionsin' into 'test-2024-black-hole-disk-interactions-in-magnetically-arrested-active-galactic-nuclei-general-relativistic-magnetohydrodynamic-simulations-using-a-time-dependent-binary-metric'

Without:

 papis rename -arb "black hole"
[INFO] commands.rename: Renaming 'test-2024-how-does-information-emerge-from-a-black-hole' into 'Test 2024 How does information emerge from a black hole?'
[INFO] commands.rename: Renaming 'test-2024-thermodynamics-and-quasinormal-modes-of-the-dymnikova-black-hole-in-higher-dimensions' into 'Test 2024 Thermodynamics and Quasinormal Modes of the Dymnikova Black Hole in Higher Dimensions'
[INFO] commands.rename: Renaming 'test-2024-black-hole-disk-interactions-in-magnetically-arrested-active-galactic-nuclei-general-relativistic-magnetohydrodynamic-simulations-using-a-time-dependent-binary-metric' into 'Test 2024 Black Hole-Disk Interactions in Magnetically Arrested Active Galactic Nuclei: General Relativistic Magnetohydrodynamic Simulations Using A Time-Dependent, Binary Metric'

Picking files:

 papis rename -rbs "black hole"
#How does information emerge from a black hole?
  Raju, Suvrat
   (2024) []
#Thermodynamics and Quasinormal Modes of the Dymnikova Black Hole in Higher Dimensions
  Macêdo, M. H. and Furtado, J. and Alencar, G. and Landim, R. R.
   (2024) []
|Black Hole-Disk Interactions in Magnetically Arrested Active Galactic Nuclei: General Relat
| Ressler, Sean M. and Combi, Luciano and Li, Xinyu and Ripperda, Bart and Yang, Huan
|  (2024) []

[INFO] commands.rename: Renaming 'Test 2024 How does information emerge from a black hole?' into 'test-2024-how-does-information-emerge-from-a-black-hole'
[INFO] commands.rename: Renaming 'Test 2024 Thermodynamics and Quasinormal Modes of the Dymnikova Black Hole in Higher Dimensions' into 'test-2024-thermodynamics-and-quasinormal-modes-of-the-dymnikova-black-hole-in-higher-dimensions'

What do you think?

kiike · 2024-04-10T09:28:01Z

While the new features work fine during testing with my live library, they don't behave properly during automatic testing. I am trying to test via the CLI to improve coverage and parse the logs but the behaviour of run is very strange, where half of the operations fail.

tests/commands/test_rename.py

This avoids conflicts when running under Pytest

tests/commands/test_rename.py

kiike · 2024-04-11T13:33:39Z

I went ahead and added some more tests too. It's at 98% coverage now.

EDIT: I need to monkeypatch something else too. I'll start the windows machine to review the changes there.
EDIT2: That did it. This is good to go after I document the changes unless of course you have more feedback 😄

alexfikl

Thank you for working on this! ❤️ This is definitely a nice thing to have, since rename was very limited before, but the interface needs a bit of work.

I've left a few comments with suggestions and questions.

papis/commands/rename.py

tests/commands/test_rename.py

kiike · 2024-04-11T19:29:01Z

Thank you for working on this! ❤️ This is definitely a nice thing to have, since rename was very limited before, but the interface needs a bit of work.

I've left a few comments with suggestions and questions.

Thank you very much for your feedback! Let me address the changes! If you feel brave, you can check if it fails on your end too in pytest if you don't have prepare_run. Yesterday, when I tried it, the document list was being manipulated while running and I couldn't trace the reason.

Co-authored-by: Alex Fikl <alexfikl@gmail.com>

jghauser · 2024-04-11T20:20:15Z

First of all, great contribution! ❤️

I just started reviewing this but then I realised that things are changing under my nose and maybe it's not quite yet the moment for detailed feedback on the documentation. I'm happy to take a look at the docs again once things stabilise 😅.

So I just have the following comments:

I noticed a warning when pressing enter to accept the default name. I think that's a bit misleading as it's the expected behaviour.
I agree with @alexfikl that simplifying the command by removing the slugify flag is a good idea (but I just saw that you've already changed this! 🎉).
I also like the proposal regarding making --regenerate into --folder-name with a functionality in line with other commands
I think we should note somewhere that the --batch flag only does something in conjunction with the --regenerate flag (mostly a note to myself for the later docs feedback)

kiike · 2024-04-11T21:09:59Z

@jghauser you can test it now if you want!

EDIT: I don't know if that will affect you, but I am going to test pushing a couple tweaks for the git section in run. It fails on real Windows. No mv command there by default, so I'll use shutil.
EDIT2: I tried the fixes on windows. It doesn't fail anymore. I'll check if there are more "unixy" commands around that would be run on Windows.

alexfikl · 2024-04-12T08:27:25Z

@kiike I looked through the code a bit and moved some things around. It all looks good to me now! 🎉
Let me know if you spot anything fishy in what I added!

If you feel brave, you can check if it fails on your end too in pytest if you don't have prepare_run. Yesterday, when I tried it, the document list was being manipulated while running and I couldn't trace the reason.

Yeah, it fails on my system too 😢 I looked a bit, but not sure why it's doing that.. best guess is that the documents in the database are stored in a mutable list and mutable references are returned on the query. We can leave it as is for now!

EDIT: I don't know if that will affect you, but I am going to test pushing a couple tweaks for the git section in run. It fails on real Windows. No mv command there by default, so I'll use shutil.

That's a good change either way! We should be using the more platform independent version from shutil.

Sure. Not a huge fan of --folder-name since what you're providing is a pattern. Unless the pattern is actually a name (doesn't include variables). Then it just gets confusing.

I'm not sure I understand why you're not a fan of the --folder-name?

alexfikl

The code all looks good to me now!

@jghauser If you get the chance to look over the docs 😁

kiike · 2024-04-12T08:41:58Z

The code all looks good to me now!

@jghauser If you get the chance to look over the docs 😁

Yay! Thanks for reviewing it! Let's see what @jghauser thinks!

jghauser · 2024-04-13T09:30:39Z

I was just looking through the docs and writing up some suggestions regarding the --folder-name option. I think the suggestions by @alexfikl and I might have ended up messing things up a bit 😅.
Before, running the command without --regenerate would give you the manual renaming option with the existing name as the default. Now, the command without --folder-name will rename things according to the settings in add-folder-name, and so it's really the same the --regenerate option (because by default that's what it's set to). Is that expected? I preferred the earlier functionality (especially because add-folder-name is empty by default so that the default folder name is an empty string).

I can see two solutions:

Having a --regenerate and a --folder-name option and never set --folder-name to add-folder-name
Retain the current behaviour, but set the default folder name to the previous name if add-folder-name isn't defined

What do you think?

alexfikl · 2024-04-13T09:36:12Z

I can see two solutions:

Having a --regenerate and a --folder-name option and never set --folder-name to add-folder-name

Retain the current behaviour, but set the default folder name to the previous name if add-folder-name isn't defined

What do you think?

Oops, I forgot about the --regenerate flag! I vote for the second option, maybe with a little warning that they need to set the option if it's not set or the rename won't do anything.

For comparison, in papis add, if it gets an empty folder from add-folder-name, it falls back to MD5HASH-author-something.

jghauser · 2024-04-13T09:39:04Z

Oops, I forgot about the --regenerate flag! I vote for the second option, maybe with a little warning that they need to set the option if it's not set or the rename won't do anything.

For comparison, in papis add, if it gets an empty folder from add-folder-name, it falls back to MD5HASH-author-something.

Yes that seems sensible. It would keep the amounts of flags small and is in line with how papis add works!

jghauser · 2024-04-13T10:02:00Z

I've finished up with the docs (assuming the functionality just discussed). I'm just gonna post the whole thing here as that's easier (especially cause the code might still change a bit) and I'll be away for a few days over the weekend. I've added a small paragraph after the intro sentence, because I think that's useful information specific to this command (and applicable to almost all examples below).

The ``rename`` command is used to rename document folders based on user input,
a provided formatting pattern, or :confval:`add-folder-name`.

It will (except when run with ``--all``) bring up the picker with a list of
documents that match the query. In the picker, you can select one or more
documents and then initiate renaming by pressing enter. Folder names are cleaned,
so that various characters (white spaces, punctuation, capital letters, and some
others) are automatically converted.

Examples
^^^^^^^^

- Rename folders for documents whose author is "Rick Astley". You can then either
  enter a new folder name or accept Papis' suggestion. The suggested folder name
  will be generated according to :confval:`add-folder-name` or, if this option
  isn't set, the current folder name.

   .. code:: sh

        papis rename author:"Rick Astley"

- You can use ``--folder-name`` to pass in your desired name. This option
  supports Papis formatting patterns. You will be asked for confirmation
  before the folder is renamed.

   .. code:: sh

        papis rename --folder-name "{doc[author]}-never-gonna" author:"Rick Astley"

  This will give you a folder named "rick-astley-never-gonna".


- To stop Papis from asking for confirmation, use the ``--batch`` flag:

   .. code:: sh

        papis rename --batch author:"Rick Astley"

- If you want to rename all documents without narrowing down your selection in
  the picker, you can use the ``--all`` flag. Be careful when combining this
  with ``--batch``, as you might end up renaming a lot folders without
  confirmation:

   .. code:: sh

        papis rename --all author:"Rick Astley"

kiike · 2024-04-13T13:41:58Z

@kiike I looked through the code a bit and moved some things around. It all looks good to me now! 🎉 Let me know if you spot anything fishy in what I added!

Nice! absolutely sure because I am not looking closely at the git code, but you requested me to use mv_and_commit_resource in #816 but would that work in both following scenarios?

- file is tracked by git.
- file is moved via papis mv (using papis.utils.git.mv)
- git makes an automatic commit with a templated message, but we're commiting anyway.
- is the repo clean?
- file is untracked
- file is moved via papis mv
- a commit is done
- repo is clean (haven't tested yet)

Yeah, it fails on my system too 😢 I looked a bit, but not sure why it's doing that.. best guess is that the documents in the database are stored in a mutable list and mutable references are returned on the query. We can leave it as is for now!

Yeah, I couldn't pinpoint the source of the problem since that only happened during pytest. We could deepcopy documents, but I'm not sure if that will be helpful at all. Now that you are pushing to this branch, you can do it, or I can. Advise please 😄.

That's a good change either way! We should be using the more platform independent version from shutil.

Good! I'll keep an eye for more raw commands!

I'm not sure I understand why you're not a fan of the --folder-name?

If it was folder-pattern or folder-format or equivalent that'd be clearer for the purpose of that flag. Especially since it says in the help folder-name (Papis format). Now that I think of it, another unintuitive config option is header-format, since that's not for a header but for an entry in the picker.

∙ ∙ Co-authored-by: Julian Hauser <julian@julianhauser.com>

kiike · 2024-04-13T14:13:09Z

I'm adding the docstring shared by @jghauser. Thanks for the edits @alexfikl and @jghauser! ❤️

alexfikl · 2024-04-13T16:41:11Z

would that work in both following scenarios?

As far as I can know, the the git code mostly assumes that it's all well set up and all the files exist and are tracked, so it's pretty brittle. On the bright side, it's all pretty well separated in papis.git, so we can make it smarter if needed.

Yeah, I couldn't pinpoint the source of the problem since that only happened during pytest. We could deepcopy documents, but I'm not sure if that will be helpful at all. Now that you are pushing to this branch, you can do it, or I can. Advise please 😄.

Doing a deep copy seems very expensive, so I'm not a big fan. For now this is fine. Maybe open an issue about it so we keep track?

If it was folder-pattern or folder-format or equivalent that'd be clearer for the purpose of that flag. Especially since it says in the help folder-name (Papis format). Now that I think of it, another unintuitive config option is header-format, since that's not for a header but for an entry in the picker.

Ah, fair enough! Completely agree on all of those points! We should probably go over those at some point and choose slightly better names, but it's tedious, so not sure it's gonna be on the top of the list :(

For now, I'm thinking consistency between commands is at least good to have.

alexfikl · 2024-04-13T16:44:57Z

This is good to go from my end, so we can get it in!
@kiike Everything ok for you?

kiike · 2024-04-14T08:32:28Z

would that work in both following scenarios?

As far as I can know, the the git code mostly assumes that it's all well set up and all the files exist and are tracked, so it's pretty brittle. On the bright side, it's all pretty well separated in papis.git, so we can make it smarter if needed.

Got it! I am not using git for my library but probably should. I'll give it a go sometime!

Yeah, I couldn't pinpoint the source of the problem since that only happened during pytest. We could deepcopy documents, but I'm not sure if that will be helpful at all. Now that you are pushing to this branch, you can do it, or I can. Advise please 😄.

Doing a deep copy seems very expensive, so I'm not a big fan. For now this is fine. Maybe open an issue about it so we keep track?

Sure, I'll create an issue for that! Spelunking into the code to see why it's being mutated will be interesting!

If it was folder-pattern or folder-format or equivalent that'd be clearer for the purpose of that flag. Especially since it says in the help folder-name (Papis format). Now that I think of it, another unintuitive config option is header-format, since that's not for a header but for an entry in the picker.

Ah, fair enough! Completely agree on all of those points! We should probably go over those at some point and choose slightly better names, but it's tedious, so not sure it's gonna be on the top of the list :(

I agree. Naming is hard 😄

For now, I'm thinking consistency between commands is at least good to have.

Definitely!

This is good to go from my end, so we can get it in!
@kiike Everything ok for you?

I think so! It has received a good makeover. I am happy with the functionality now and the features added by you too regarding git!

alexfikl · 2024-04-14T08:40:43Z

Thanks for working on this! In it goes 🚀

feat(rename): allow batch and auto-renaming from pattern

5352a08

kiike force-pushed the feat/batch_auto_rename branch from e0ff4b4 to 5352a08 Compare April 10, 2024 09:26

alexfikl reviewed Apr 10, 2024

View reviewed changes

tests/commands/test_rename.py Outdated Show resolved Hide resolved

tests/commands/test_rename.py Outdated Show resolved Hide resolved

tests(rename): use own runner and remove unnecessary config fixture

bce8e7a

kiike marked this pull request as ready for review April 10, 2024 14:37

kiike force-pushed the feat/batch_auto_rename branch 5 times, most recently from 9c75822 to 54d68fc Compare April 10, 2024 17:42

fix(tests/rename): collect all renames before running

25aa232

This avoids conflicts when running under Pytest

kiike force-pushed the feat/batch_auto_rename branch from 54d68fc to 25aa232 Compare April 10, 2024 18:26

tests(rename): skip CLI tests on Windows.

c8b6088

alexfikl reviewed Apr 11, 2024

View reviewed changes

tests/commands/test_rename.py Outdated Show resolved Hide resolved

kiike force-pushed the feat/batch_auto_rename branch from c915cb9 to 4db6e34 Compare April 11, 2024 13:45

tests(rename): fix testing on windows and add more tests

f8d3290

kiike force-pushed the feat/batch_auto_rename branch from 4db6e34 to 37264eb Compare April 11, 2024 14:35

docs(rename): document the new flags

641f74e

kiike force-pushed the feat/batch_auto_rename branch from 37264eb to 641f74e Compare April 11, 2024 14:36

kiike requested a review from alexfikl April 11, 2024 14:39

alexfikl reviewed Apr 11, 2024

View reviewed changes

kiike force-pushed the feat/batch_auto_rename branch 2 times, most recently from b27cd5c to 1012bf4 Compare April 11, 2024 19:47

kiike and others added 2 commits April 11, 2024 21:51

fix(rename): remove slugify for consistency with other commands

be71b9f

fix(rename): apply suggestions from code review

04c17f1

Co-authored-by: Alex Fikl <alexfikl@gmail.com>

kiike force-pushed the feat/batch_auto_rename branch from 1012bf4 to 04c17f1 Compare April 11, 2024 19:51

kiike requested a review from alexfikl April 11, 2024 21:05

refactor(rename): use --folder-name and behave like add

78839e2

kiike force-pushed the feat/batch_auto_rename branch from 2820a1c to 78839e2 Compare April 11, 2024 21:09

kiike added 2 commits April 11, 2024 23:30

refactor(rename): move imports for run into run

f7eb586

fix(rename): use shutil.move for moving instead of mv

d75d421

kiike force-pushed the feat/batch_auto_rename branch from 978bc4e to d75d421 Compare April 11, 2024 21:32

alexfikl added 3 commits April 12, 2024 09:43

git: add mv command helpers

2720887

tests: update rename tests

37d5c66

commands.rename: add note for separate run

8c8bfdd

alexfikl approved these changes Apr 12, 2024

View reviewed changes

kiike mentioned this pull request Apr 12, 2024

tests(windows): fix remaining "unixisms" that break some tests #816

Merged

docs(rename): add revised docstring

68e0b4f

∙ ∙ Co-authored-by: Julian Hauser <julian@julianhauser.com>

commands.rename: add warning for missing folder-name

1108ded

alexfikl force-pushed the feat/batch_auto_rename branch from 362ebac to 1108ded Compare April 13, 2024 16:58

alexfikl merged commit 9f315d7 into papis:main Apr 14, 2024
19 checks passed

kiike mentioned this pull request Apr 14, 2024

Resulting document list from papis.cli.handle_doc_folder_query_sort being sneakily mutated during iteration #818

Open

kiike deleted the feat/batch_auto_rename branch April 14, 2024 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rename): allow batch and auto-renaming from pattern #810

feat(rename): allow batch and auto-renaming from pattern #810

kiike commented Apr 10, 2024 •

edited

Loading

kiike commented Apr 10, 2024

kiike commented Apr 11, 2024 •

edited

Loading

alexfikl left a comment

kiike commented Apr 11, 2024

jghauser commented Apr 11, 2024

kiike commented Apr 11, 2024 •

edited

Loading

alexfikl commented Apr 12, 2024

alexfikl left a comment

kiike commented Apr 12, 2024

jghauser commented Apr 13, 2024

alexfikl commented Apr 13, 2024 •

edited

Loading

jghauser commented Apr 13, 2024

jghauser commented Apr 13, 2024 •

edited

Loading

kiike commented Apr 13, 2024 •

edited

Loading

kiike commented Apr 13, 2024

alexfikl commented Apr 13, 2024

alexfikl commented Apr 13, 2024 •

edited

Loading

kiike commented Apr 14, 2024

alexfikl commented Apr 14, 2024

feat(rename): allow batch and auto-renaming from pattern #810

feat(rename): allow batch and auto-renaming from pattern #810

Conversation

kiike commented Apr 10, 2024 • edited Loading

kiike commented Apr 10, 2024

kiike commented Apr 11, 2024 • edited Loading

alexfikl left a comment

Choose a reason for hiding this comment

kiike commented Apr 11, 2024

jghauser commented Apr 11, 2024

kiike commented Apr 11, 2024 • edited Loading

alexfikl commented Apr 12, 2024

alexfikl left a comment

Choose a reason for hiding this comment

kiike commented Apr 12, 2024

jghauser commented Apr 13, 2024

alexfikl commented Apr 13, 2024 • edited Loading

jghauser commented Apr 13, 2024

jghauser commented Apr 13, 2024 • edited Loading

kiike commented Apr 13, 2024 • edited Loading

kiike commented Apr 13, 2024

alexfikl commented Apr 13, 2024

alexfikl commented Apr 13, 2024 • edited Loading

kiike commented Apr 14, 2024

alexfikl commented Apr 14, 2024

kiike commented Apr 10, 2024 •

edited

Loading

kiike commented Apr 11, 2024 •

edited

Loading

kiike commented Apr 11, 2024 •

edited

Loading

alexfikl commented Apr 13, 2024 •

edited

Loading

jghauser commented Apr 13, 2024 •

edited

Loading

kiike commented Apr 13, 2024 •

edited

Loading

alexfikl commented Apr 13, 2024 •

edited

Loading