Skip to content

some fixes - #38

Open
leonclifford wants to merge 8 commits into
lunarcloud:mainfrom
leonclifford:main
Open

leonclifford wants to merge 8 commits into
lunarcloud:mainfrom
leonclifford:main

Conversation

@leonclifford

Copy link
Copy Markdown

Changes i made:
script-dialog.sh: Sources are gathered automatically from folder to avoid source issues and added function call from command: "$ ./script-dialog datepicker" in CLI calls datepicker function.

datepicker.sh: Replaced month=number if chain by an array that contains month and it's number associated: Jan[1].

Comment thread helpers.sh
# Variables set in init.sh and used here
# shellcheck disable=SC2154

function listfunc() {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This function is just an alias without any "if platform do a, else do b" logic and has no documentation comments. Doesn't belong in this library.

Comment thread script-dialog.sh
# LGPL-2.1 license

# Get the directory where this script is located
# get the directories

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This comment change doesn't make sense. Only one directory is being gathered by the following line(s) still

Comment thread script-dialog.sh
# shellcheck source=./init.sh
source "${SCRIPT_DIALOG_DIR}/init.sh"
# sources everything in folder, excludes current file
for src in ./*.sh

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I would prefer a finite list rather than an unbounded "all in folder" for obvious security reasons

@lunarcloud

Copy link
Copy Markdown
Owner

Not seeing where the "call function from script" logic is but I'm not sure it's preferable to the "source script-dialog; datepicker" workflow. But the months as array lookup is a nice one. Maybe not bundling an api change and a code cleanup together would be best.

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