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(grimshot): modularize and clean up with functional helpers #38

Merged
merged 3 commits into from
Oct 7, 2024

Conversation

Qubut
Copy link
Contributor

@Qubut Qubut commented Sep 7, 2024

  • Introduced functional helper methods: when, if_else, and any for cleaner, more maintainable logic.
  • Refactored conditional handling in notifyOk, notifyError, and check functions to leverage the new helper methods.
  • Moved usage message printing to a dedicated printUsageMsg function.
  • Extracted subject-specific logic (e.g., area, window, screen) into standalone functions: selectArea, selectWindow, etc.
  • Improved error handling by introducing functions like handleScreenshotSuccess, handleScreenshotFailure, and handleSaveCopy.
  • Reformatted code and improved readability by using consistent formatting for case statements and conditions.

This refactor enhances code maintainability by isolating logic into reusable components, improving clarity and simplifying future changes.

- Introduced functional helper methods: `when`, `if_else`, and `any` for cleaner, more maintainable logic.
- Refactored conditional handling in `notifyOk`, `notifyError`, and `check` functions to leverage the new helper methods.
- Moved usage message printing to a dedicated `printUsageMsg` function.
- Extracted subject-specific logic (e.g., area, window, screen) into standalone functions: `selectArea`, `selectWindow`, etc.
- Improved error handling by introducing functions like `handleScreenshotSuccess`, `handleScreenshotFailure`, and `handleSaveCopy`.
- Reformatted code and improved readability by using consistent formatting for case statements and conditions.

This refactor enhances code maintainability by isolating logic into reusable components, improving clarity and simplifying future changes.
@OctopusET
Copy link
Owner

Thank you for your work, I will check it when I have some time.
I think if we are going to refactor this, we can move grimshot related files into ./grimshot directory. Would you do that?

@Qubut
Copy link
Contributor Author

Qubut commented Sep 11, 2024

yes it is a good idea, but I suggest that the functional_helpers file be in a folder called utils or scripts/functions or sth. like that
since the utilities can be used in other future scripts.

@Qubut
Copy link
Contributor Author

Qubut commented Sep 11, 2024

I've also another question, I noticed that the grim-completion script is a bash script while the grim script is a Bourne shell script
why isn't grim also a bash script ?

@OctopusET
Copy link
Owner

grimshot-completion script is added later, and someone could make zsh or fish completion script for grimshot.

@OctopusET
Copy link
Owner

I think modularization is good, but having two files won't be good for this case. Having one script file would be better to install grimshot script. So, I think it would be better to merge two files into one.

- Moved all grim related files to their own directory
- Refactored argument parsing into the `parseArgs` function for improved
clarity.
- Added `handleUnknownSubject` to handle unknown subjects explicitly.
- Introduced `POSITIONAL_ARGS` to collect and reassign positional
arguments using `set -- $POSITIONAL_ARGS` after flag parsing, ensuring
correct positional argument handling.
- Ensured flag parsing (`--notify`, `--cursor`, `--wait`) is processed
before positional arguments.
- Stored `$1` as `error_message` in `notifyError` to fix incorrect
reference during condition evaluation.
@Qubut
Copy link
Contributor Author

Qubut commented Sep 12, 2024

Thank you for your work, I will check it when I have some time. I think if we are going to refactor this, we can move grimshot related files into ./grimshot directory. Would you do that?

I moved all related grimshot files into ./grimshot directory as requested

also for modularity I refactored argument parsing into it's own func. and added some fixes

@Qubut
Copy link
Contributor Author

Qubut commented Sep 30, 2024

@OctopusET Any update about this PR?

Copy link
Owner

@OctopusET OctopusET left a comment

Choose a reason for hiding this comment

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

LGTM

@OctopusET OctopusET merged commit e2c50d7 into OctopusET:master Oct 7, 2024
1 check passed
@OctopusET
Copy link
Owner

Sorry for delay! @Qubut

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