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.

Sign up for GitHub

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

Jump to bottom

Suggestions for v1.0.4 #1

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Suggestions for v1.0.4 #1

wants to merge 12 commits into from

Conversation

loelkes
Copy link

@loelkes loelkes commented Jan 2, 2021

  • Replace stat with openssl md5 for better compatibility
  • Introduce -v | --version CLI argument
  • Parse CLI arguments before checks so -h | --help or -v | --version can be used anytime.
  • Update README.md

@loelkes
replace stat with openssl md5
e10afe2 
Command line arguments for stat are different on macOS (Darwin) and
Linux. If the file does not end with a empty line some editors (like
nano) will add one even if you save the file without any modifications.
In this case, the md5 hash is different even if the user did not change
any content. Adding a newline is a workaround for this.
@loelkes
place checks after the CLI parser so that -h works anytime.
1b304ab
@loelkes
add -v --version to show the current version to the CLI
08d48ce
@loelkes
only show information about $SETTINGS_FILE if it exists
b40d863
@loelkes
wrap long if else lines for readability
18bfcc9
@loelkes
remove homebrew instructions, add CLI argument documentation
b5c0762
@loelkes
only show changes to apply if DEBUG is enabled
9ff7089
@t0r0X
Copy link
Owner

t0r0X commented Jan 4, 2021

Good suggestions, thanks!

t0r0X
t0r0X previously approved these changes Jan 4, 2021
View reviewed changes
Copy link
Owner

@t0r0X t0r0X left a comment •
edited
Loading

Choose a reason for hiding this comment

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

In my opinion:
e10afe2 looks good
1b304ab looks good
08d48ce ok with change request, see inline comment
b40d863 ok with change request, see inline comment
18bfcc9 look good
b5c0762 looks good (I misunderstood it at first reading)
9ff7089 looks good

t0r0X
t0r0X requested changes Jan 4, 2021
View reviewed changes
git-redate Outdated Show resolved Hide resolved
git-redate Outdated Show resolved Hide resolved
@t0r0X t0r0X dismissed their stale review January 4, 2021 09:25

The approval was only for first commit, I didn't know it applies to the whole PR.

@t0r0X
Copy link
Owner

t0r0X commented Jan 4, 2021

Thanks again for the changes/fixes/extensions! I'd be more than happy to approve the PR. Please could you take a look at the changes I suggested?

@loelkes loelkes requested a review from t0r0X January 4, 2021 12:27
@loelkes
Copy link
Author

loelkes commented Mar 11, 2021

@t0r0X I made the changes you requested. Can you take a look?

@loelkes
Copy link
Author

loelkes commented Apr 24, 2021

Poke @t0r0X

@TemaSM
Copy link

TemaSM commented Apr 26, 2022

image

Any updates here? @t0r0X please take a look 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t0r0X t0r0X Awaiting requested review from t0r0X

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@loelkes @t0r0X @TemaSM