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

convert: add option to symlink instead of copying #2326

Closed
wants to merge 1 commit into from
Closed

convert: add option to symlink instead of copying #2326

wants to merge 1 commit into from

Conversation

kierdavis
Copy link
Contributor

As proposed in #2324.

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I left just a few suggestions for the documentation.

transcoding, instead of copying them; \
this may cause files in your library to \
be modified, if options such as "embed" \
are enabled')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The in-line help string should usually be shorter than this—just one phrase is fine. We can leave the full detail for the documentation.

step will cause the original files to be modified as well if ``link`` is
enabled. For this reason, it is highly recommended not use to ``link`` and
``embed`` at the same time.
Default: ``false``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some identical text between this and the -l option. For maintainability, could you please just leave one copy and refer to one from the other? For example, you might just say "see the description of the link option below" or something.

@kierdavis
Copy link
Contributor Author

@sampsyo Thanks for the quick feedback. I did think it felt quite lengthy when I wrote it, but I wanted to ensure users are aware that embed (which is on by default) and link used together may cause the original files to be modified. Maybe a better solution would be to add an "are you sure?" prompt in the code when this combination of options is detected; what do you think?

@sampsyo
Copy link
Member

sampsyo commented Dec 17, 2016

Sure! Or here are two other ideas:

  • Just a short phrase like "(don't use with embed, probably)" as a warning there.
  • The plugin could just refuse to run with both flags enabled—I doubt anyone will actually want that behavior.

Vexatos added a commit to Vexatos/beets that referenced this pull request Aug 20, 2019
As proposed in beetbox#2324.

Updated commit from beetbox#2326.

Co-authored-by: Vexatos <[email protected]>
Vexatos added a commit to Vexatos/beets that referenced this pull request Aug 20, 2019
As proposed in beetbox#2324.

Updated commit from beetbox#2326.

Co-authored-by: Vexatos <[email protected]>
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.

2 participants