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

Feature-gate RAR support #566

Merged
merged 4 commits into from
Dec 15, 2023
Merged

Feature-gate RAR support #566

merged 4 commits into from
Dec 15, 2023

Conversation

cyqsimon
Copy link
Contributor

@cyqsimon cyqsimon commented Dec 3, 2023

See #565.

This PR does not fully close the issue; the ideal solution would be to use a FOSS library for unrar support. But that can wait.

Unresolved/todo

  • Should unrar feature be opt-in or opt-out?
    • I personally prefer opt-in, again because of unrar's unfriendly license.
  • Update CI to test both cases (with unrar feature enabled and disabled).
  • Changelog entry.

@cyqsimon cyqsimon marked this pull request as draft December 3, 2023 16:28
tests/ui.rs Outdated Show resolved Hide resolved
tests/ui.rs Show resolved Hide resolved
@marcospb19
Copy link
Member

  • Should unrar feature be opt-in or opt-out?
  • I personally prefer opt-in, again because of unrar's unfriendly license.

I'd prefer if it was opt-out, here's how I think about it:

  • If you have a restriction in your distribution, you can restrict this feature so you're good 👍.

Instead of:

  • If you don't have any restrictions related to this problem that other people might have, you can add this feature.

I think the first one seems more natural, plus, Ouch is licensed under MIT so people should be aware that non-FOSS code might be in it, at least if that's a dealbreaker for them.

@cyqsimon
Copy link
Contributor Author

cyqsimon commented Dec 9, 2023

I'd prefer if it was opt-out, here's how I think about it:

  • If you have a restriction in your distribution, you can restrict this feature so you're good 👍.

Yeah that's fine. I'm happy to respect project owners' decision on this.

@cyqsimon
Copy link
Contributor Author

cyqsimon commented Dec 9, 2023

Rebased.

@marcospb19
Copy link
Member

marcospb19 commented Dec 9, 2023

Hi! Is it ready for review?

PR is set as draft.

@cyqsimon
Copy link
Contributor Author

cyqsimon commented Dec 9, 2023

Hi! Is it ready for review?

PR is set as draft.

Not yet. Still have to modify CI (which could really use some cleaning up). I'll also try to reenable tests on Windows and see what's wrong there.

Will make some time for this tomorrow.

@cyqsimon cyqsimon mentioned this pull request Dec 10, 2023
@cyqsimon
Copy link
Contributor Author

Blocking on #578.

@marcospb19
Copy link
Member

Yep sorry for that delay, I'll answer you as soon as feasible.

@cyqsimon
Copy link
Contributor Author

cyqsimon commented Dec 14, 2023

Rebased on main.

@cyqsimon cyqsimon marked this pull request as ready for review December 14, 2023 10:53
Copy link
Member

@marcospb19 marcospb19 left a comment

Choose a reason for hiding this comment

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

Thanks for this, I gave it a deep review but found no flaws.

I also appreciate your help with the CI and with how to use insta with conditional compilation, I never thought of changing the test name (I didn't know it was possible, hard to find it in the docs).

@marcospb19 marcospb19 merged commit 036e120 into ouch-org:main Dec 15, 2023
82 checks passed
@cyqsimon cyqsimon deleted the rar-feature branch December 21, 2023 10:29
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