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

Refactor utility functions #960

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jeevithakannan2
Copy link
Contributor

Type of Change

  • Refactoring

Description

  • Separate utitility functions into user manager functions and monitor control functions so that xorg will not be installed when user account manager is used.
  • Disable some shell checks and formatting
  • Remove redundant function calls

Testing

  • Tested on arch linux without issues.

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.
  • My changes generate no errors/warnings/merge conflicts.

Separate utitility functions into user manager functions and monitor control functions, disable somechecks and formatting
@adamperkowski adamperkowski added refactor script Pull requests that update scripts labels Nov 22, 2024
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.

I don't feel like this is necessary. The changes here don't really improve anything.

@jeevithakannan2
Copy link
Contributor Author

jeevithakannan2 commented Nov 22, 2024

I don't feel like this is necessary. The changes here don't really improve anything.

Currently when user tries to use the user account manager commands it installs xrandr from the utility functions script. This PR separates monitor functions and user manager functions so that xrandr is not installed when trying to use user manager scripts. ( Already mentioned in PR description )

@adamperkowski
Copy link
Collaborator

adamperkowski commented Nov 22, 2024

Currently when user tries to use the user account manager commands it installs xrandr from the utility functions script. This PR separates monitor functions and user manager functions so that xrandr is not installed when trying to use user manager scripts. ( Already mentioned in PR description )

Is this even an issue though?
I feel like structuring scripts like this would only make the codebase more confusing and new first-time contributions potentially harder.

@jeevithakannan2
Copy link
Contributor Author

jeevithakannan2 commented Nov 22, 2024

Is this even an issue though?

Yes it is a wayland user getting xorg, xrandr installed automatically when trying to use the user manager is a problem.

I feel like structuring scripts like this would only make the codebase more confusing and new first-time contributions potentially harder.

How separating functions into their own respective module makes it confusing? Monitor functions will be inside the monitor directory and user functions will be inside user directory. Right now having two confirm actions functions and all the monitor utility related code and user manager related code in same file is more confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor script Pull requests that update scripts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants