style: Fix generate script to confirm to shell scripting conventions, which mostly includes style changes, but also makes it run on Alpine Linux

MetabolicAtlas / data-generation

Process the raw data-files for ingestion into the Neo4j database

MIT License

0 stars 0 forks source link

style: Fix generate script to confirm to shell scripting conventions, which mostly includes style changes, but also makes it run on Alpine Linux #34

Closed kusalananda closed 1 year ago

kusalananda commented 1 year ago

Mostly small style changes to the generate shell script.

One of the changes, changing the names of the option used with sort, makes it run on systems with Busybox sort, e.g. Alpine Linux (this fixes #35).

A summary of changes:

Script does not use bash-isms, so use /bin/sh
Quoting not needed in initial variable assignments.
Reformat function.
Use options with sort that are more commonly supported.
Don't test $? separately.
Don't call exit in function (return instead).
Do exit from outer scope if function retuns non-zero.
Use printf for variable data.
Don't use curly braces when expanding variables.
Use default path also when argument is empty (not just unset).

Testing

[x] build the stack without errors
[x] all tests should pass

mihai-sysbio commented 1 year ago

It would be nice with a more descriptive PR name, also in the style of the conventional commits (style: ...). Any clue as to why the side-by-side diff shows everything has changed?

kusalananda commented 1 year ago

It would be nice with a more descriptive PR name, also in the style of the conventional commits (style: ...). Any clue as to why the side-by-side diff shows everything has changed?

PR title has been updated to be more descriptive.

I assume that a side-by-side diff would show that everything has changed if I managed to actually touch everything. If you want a breakdown of the changes with individual descriptions, these are found in the individual commits and their attached commit messages.

Is it convention to repeat the commit messages in the PR?

mihai-sysbio commented 1 year ago

Is it convention to repeat the commit messages in the PR?

If there is only one commit in the PR, that's actually what GitHub does by default. To answer your question - not quite repeat, but summarize. That tends to yield a title closer to the issue title.

mihai-sysbio commented 1 year ago

I went ahead and amended the issue body with a Testing section, please feel free to edit it.

mihai-sysbio commented 1 year ago

Assuming the work in the issue has been completed, I've gone ahead and asked for reviews hoping that this PR can be merged in this sprint.