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

chore: Remove unwanted case #757

Closed

Conversation

jeevithakannan2
Copy link
Contributor

@jeevithakannan2 jeevithakannan2 commented Oct 4, 2024

Type of Change

  • New feature
  • Bug fix
  • Documentation update
  • Refactoring
  • Hotfix
  • Security patch
  • UI/UX improvement

Description

  • Unsupported package manager is already handled in common-script.sh
    printf "%b\n" "${RED}Can't find a supported package manager${RC}"
  • Removed pacman from linutil-installer.sh cargo installation section as pacman case is already handled.
  • Remove snaps script is excluded as it may conflict with refact: remove snaps #756

Testing

  • No errors works as expected.

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no errors/warnings/merge conflicts.

Copy link
Collaborator

@adamperkowski adamperkowski left a comment

Choose a reason for hiding this comment

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

NIT: I'm not sure if every script supports every "officially" supported manager, If not, we need to fix this.

@lj3954
Copy link
Contributor

lj3954 commented Oct 5, 2024

What if a different package manager is added? This PR offers no improvements to maintainability or anything of the sort, and only offers the potential for issues to come up in the future.

@jeevithakannan2
Copy link
Contributor Author

If another package manager is added it will be handled anyway. For example, if we are adding nix then we will be adding nix to most of the scripts. If nix cannot be used for a specific script then it will be handled as unsupported. Having extra bloat that will not be reached is useless anyway.

@lj3954
Copy link
Contributor

lj3954 commented Oct 5, 2024

If nix cannot be used for a specific script then it will be handled as unsupported.

Yes. And this PR proposes the removal of that error handling.

@jeevithakannan2
Copy link
Contributor Author

jeevithakannan2 commented Oct 5, 2024

Having extra bloat that cannot be reached is useless anyway 🤷🏻‍♂️
We can always add them back to scripts that don't support packagers later

@lj3954
Copy link
Contributor

lj3954 commented Oct 5, 2024

Having extra bloat that cannot be reached is useless anyway 🤷🏻‍♂️ We can always add them back to scripts that don't support packagers later

They cannot be reached assuming all package managers supported by linutil are supported by the script. I've just stated a way in which that condition could be broken. Your idea of adding this code back in the case of any changes illustrates why removing it is a poor decision to begin with.

@jeevithakannan2 jeevithakannan2 deleted the package-managers branch November 1, 2024 10:01
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.

3 participants