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

📃 fix: replace every ~ AND ,, for posix compliance #666

Closed
wants to merge 5 commits into from

Conversation

adamperkowski
Copy link
Collaborator

@adamperkowski adamperkowski commented Sep 24, 2024

Type of Change

  • Bug fix
  • Refactoring

Description

Using tilde is a bashism, using $HOME is not.
Same with logic expressions and ,,s.

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my own code.
  • My changes generate no errors/warnings/merge conflicts.

(📃 is a shell script btw)

@adamperkowski adamperkowski marked this pull request as draft September 28, 2024 19:00
@adamperkowski
Copy link
Collaborator Author

More to do

@adamperkowski adamperkowski changed the title Change every ~ to $HOME in shell scripts fix: remove every ~ for posix-compliance Sep 28, 2024
@adamperkowski adamperkowski marked this pull request as ready for review September 28, 2024 19:14
@adamperkowski adamperkowski changed the title fix: remove every ~ for posix-compliance fix: remove every ~ & ,, for posix-compliance Sep 28, 2024
@adamperkowski adamperkowski changed the title fix: remove every ~ & ,, for posix-compliance fix: replace every ~ & ,, for posix compliance Sep 28, 2024
@adamperkowski
Copy link
Collaborator Author

@ChrisTitusTech In case you're wondering, this is OK to merge. I'll do the rest of archtitus in a later PR.

@adamperkowski adamperkowski changed the title fix: replace every ~ & ,, for posix compliance fix: replace every ~ AND ,, for posix compliance Oct 3, 2024
@adamperkowski adamperkowski changed the title fix: replace every ~ AND ,, for posix compliance 📃 fix: replace every ~ AND ,, for posix compliance Oct 3, 2024
@adamperkowski
Copy link
Collaborator Author

Clearing up confusion here:

  • using =~ is one heck of a bashism. can be avoided very easily
  • ~/ is pretty obvious. it's an alias to $HOME. a part of GNU core utils
  • "${foo,,}" =~ ^[a-z][a-z0-9_.-]{0,62}[a-z0-9]$ has like 3+ bashisms so changing it to tr '[:upper:]' '[:lower:]' is completely fine. tr is much easier to understand, too

this PR explicitly cleans up some bashisms. logic remains unchanged (excluding this below, it was only y before, I added Y)

if [[ "$force" == "y" || "$force" == "Y" ]]

@jeevithakannan2
Copy link
Contributor

jeevithakannan2 commented Nov 5, 2024

~ to $HOME is okay but the if condition changes is not necessary as whatever you do server-setup.sh will be a bash script. The arch iso ships only bash. The sh is symlinked to bash. You need to install dash package in order for this to work as posix which cannot be done. And the shebang is bash.

@adamperkowski
Copy link
Collaborator Author

~ to $HOME is okay but the if condition changes is not necessary as whatever you do server-setup.sh will be a bash script. The arch iso ships only bash. The sh is symlinked to bash. You need to install dash package in order for this to work as posix which cannot be done. And the shebang is bash.

I am 100℅ aware. But if we want to keep it fully posix compliant, let's keep it fully posix compliant.

@jeevithakannan2
Copy link
Contributor

I am 100℅ aware. But if we want to keep it fully posix compliant, let's keep it fully posix compliant.

It's a waste even though you make it fully posix compliant

@adamperkowski adamperkowski deleted the tilda_env branch November 7, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants