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

Single sed #180

Closed
wants to merge 8 commits into from
Closed

Single sed #180

wants to merge 8 commits into from

Conversation

Dashboy1998
Copy link
Contributor

Context

  • Combines all sed command that modify /palworld/Pal/Saved/Config/LinuxServer/PalWorldSettings.ini into one command

Choices

  • Using regular expressions with back referencing so if the environmental variable is not set then value is not replace (Although technically it's replaces with itself)

Test instructions

  1. Try starting the container with any valid configuration of environmental variables set.

Checklist before requesting a review

  • I have performed a self-review of my code
  • I've added documentation about this change to the README.
  • I've not introduced breaking changes.

@thijsvanloef
Copy link
Owner

While this is a "cleaner" way of doing it, I'm not fully convinced this is a solution I want to merge.
Mainly because it adds a lot of complexity to the code, making it less readable.

Also if I make a mistake anywhere in the sed command, the whole command will fail instead of only one setting.

I need to think a little more about this one

@Dashboy1998
Copy link
Contributor Author

While this is a "cleaner" way of doing it, I'm not fully convinced this is a solution I want to merge. Mainly because it adds a lot of complexity to the code, making it less readable.

Also if I make a mistake anywhere in the sed command, the whole command will fail instead of only one setting.

I need to think a little more about this one

While it does make it slightly more difficult to understand also means you only write to the file once.

@wrjp
Copy link

wrjp commented Jan 29, 2024

just a passerby - I would rather use some loop and or a function to clean this up

@thijsvanloef
Copy link
Owner

Decided against implementing this change

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