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

Break down monolithic script file into sourced function files #241

Merged
merged 3 commits into from
Nov 26, 2017

Conversation

ThomDietrich
Copy link
Member

@ThomDietrich ThomDietrich commented Nov 26, 2017

This PR basically outsources the numerous functions available in the main file openhabian-setup.sh into different files, like openhab.sh with all openHAB specific functions. These files are then re-included using the source command. The individual script files are not intended to be executed on their own (even though it would now be easier to accomplish that).

@BClark09 your opinion would be appreciated.

Resolves #225

Signed-off-by: Thomas Dietrich [email protected] (github: ThomDietrich)

Thomas Dietrich added 3 commits November 26, 2017 15:52
Signed-off-by: Thomas Dietrich <[email protected]>
Signed-off-by: Thomas Dietrich <[email protected]>
@BClark09
Copy link
Member

Looks good! I won't be able to test it until later but I don't see why it wouldn't work as is.

I like the simplicity of including all files in the function folder within the loop and this makes sense to me for unattended installs, but is there a necessity to source it all at once before the whiptail menu? Can't the relevant file be sourced just before the function is called?

@ThomDietrich
Copy link
Member Author

I've tested it and everything worked so far.
Thanks for looking over it ;)

Can't the relevant file be sourced just before the function is called?

Excellent question... That would be quite clever, I thought about that some more and still ask myself "Why?" 😄 Each file solely contains functions and no implicitly executed logic. Hence the only reason would be some kind of resource concern and I doubt we are anywhere near critical sizes... wdyt?

@ThomDietrich
Copy link
Member Author

@BClark09 I need these changes for further tests but please add your comment and we can discuss a subsequent PR

@ThomDietrich ThomDietrich merged commit 9b5b498 into master Nov 26, 2017
@ThomDietrich ThomDietrich deleted the restructure branch November 26, 2017 20:25
@BClark09
Copy link
Member

You have a good point, I think the limited resource saving wouldn't justify the effort needed to implement it.

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