search

canonical / jhack

Chock-full of Juju hackery.
Apache License 2.0
48 stars 23 forks source link

'--recursive' option is inverted. #150

Closed Thanhphan1147 closed 1 month ago

Thanhphan1147 commented 1 month ago

Thanks for the awesome tool btw!

I noticed that the default for --recursive for jhack sync is actually True, which mean walk the directory recursively without the flag and inverse with the flag.

https://github.com/canonical/jhack/blob/6e51824c186335227136f26df4b2307238fdee43/jhack/utils/sync.py#L261-L263

We can either:

  1. remove is_flag and ask for a value
  2. Change default to False which seems to make the most sense
  3. Change the name to --non-recursive if we want to walk recursively as a default behavior

option 2 makes the most sense to me so I made this PR https://github.com/canonical/jhack/pull/151 to change it. But option 3 can work as well if we want recursive to be the default behavior of the directory walk.

PietroPasotti commented 1 month ago

The default behaviour to walk recursively is desirable, so we can watch ./lib instead of having to watch all individual charm lib folders directly, for example. So, good thinking but I'll choose 3 :) and I think it's going to be consistent if we also remove is_flag.

Thanhphan1147 commented 1 month ago

I see, that makes sense. I'll update my PR with that then