geigerzaehler / beets-alternatives

Beets plugin to manage external files
MIT License
93 stars 21 forks source link

Use relative symlinks #36

Closed daviddavo closed 5 years ago

daviddavo commented 5 years ago

Related to #34

I would simply replace absolute path symlinks with relative path ones, but I understand that a config option would be necessary to let the user select which kind of links to use

wisp3rwind commented 5 years ago

Sounds like a useful feature :tada: You're right, this should go behind a config option (maybe a linktype: [absolute|relative] option would do the trick?), and a quick test that this produces valid links would also be nice.

daviddavo commented 5 years ago

There is one question yet, should we change linktype to a boolean called relativeLinks?

Pros:

Cons:

daviddavo commented 5 years ago

Now, if we want to use linkType (relative, absolute) we only have to change the name of the config (int tests and everything) and the dictionary.

If we want it as 'boolean' it could be merged now.

The name of the setting is in snake_case because the other plugins used snake case as well

geigerzaehler commented 5 years ago

There is one question yet, should we change linktype to a boolean called relativeLinks?

As you pointed out relativeLinks seems preferable. So let’s go with it.

One thing I forgot to mention is that you should also document the configuration option in the readme.

daviddavo commented 5 years ago

There's one test missing:

If the user changes the configuration, will the links be generated again?

daviddavo commented 5 years ago

When I run it from the cli, beets automatically converts yes and no to bool, (not string) so it doesn't fall inside the choices 'yes' or 'no'.

But... If I use .get(bool()), and someone puts something that it's not yes/no, then the cli says configuration error: alternatives.android.relative_links: must be a number.

And, if someone puts relative_links: 3 it will be treated as a yes...

Other plugins like play just use it 'raw', without casting

But this causes raw: Nien to return True as it's a string...

I think we should roll back to link_type. In the future more choices could be added like hard links, or mixing linking and converting files.

wisp3rwind commented 5 years ago

I think we should roll back to link_type. In the future more choices could be added like hard links, or mixing linking and converting files.

That's why I proposed linktype instead of a boolean option. I don't feel very strong about either approach, though.

If the user changes the configuration, will the links be generated again?

I'm pretty sure that right now, they will not, and I'm not sure whether they need to: beets-alternatives doesn't detect other kinds of configuration changes (such as changing the precise convert commands; #19 is also related). Because it only involves the filesystem and not direct file accesses, checking link type might be cheap enough, though. I don't think it is really essential for this to be merged: realistically, most drastic config changes happen only when setting up beets initially.

Maybe the update command should really get a --rewrite (or --force or so) flag that would make it more thorough, but which wouldn't be a thing you run for each import/tagging operation, cf #19.

wisp3rwind commented 5 years ago

But... If I use .get(bool()), and someone puts something that it's not yes/no, then the cli says configuration error: alternatives.android.relative_links: must be a number.

According to git grep -E 'config.*bool', .get(bool) is the canonical way in beets to coerce config values to booleans. The issue is that .get(bool()) evaluates to .get(False) (bool() == False!), and because isinstance(False, int) == True, this actually gives you an integer template (see as_template in confuse) which will also accept False (or no, which YAML converts to False) and True (resp. yes) because they are, in fact, integers (I don't know whether that's Python standard or implementation, just tried that in the repl).

daviddavo commented 5 years ago

Ok, I changed it to link_types again and documented it in the readme, I think it's ready to merge

daviddavo commented 5 years ago

It's already in the config and link_types is now link_type in tests, main code and README.md

geigerzaehler commented 5 years ago

I was refering to this section in the readme.

daviddavo commented 5 years ago

Sorry about the 4 rebases, first time squashing.

Thank you for being so patient