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

New named parameters implementation #86

Merged
merged 5 commits into from
Jul 21, 2019
Merged

New named parameters implementation #86

merged 5 commits into from
Jul 21, 2019

Conversation

nkakouros
Copy link
Collaborator

As already mentioned in #45, I have taken inspiration from a nice trick I found online. I expanded upon it to create the same aliases as in v2.

This is a proposal and if you find it appropriate we could work on it to expand it to fully cover v2 aliases or introduce new ones as well. For the time being I have created the [string] and [integer] aliases.

In the code, I think it would be nice to add comments to guide future contributors. I have added some minimal ones so far. Also, contrary to the previous implementation, I named internal variables that are not leaked to the calling code with friendly names to make it easier to follow the flow. Functions, aliases, etc that will be available in the scope of the caller have been namespaced with bash_oo_<library-name>. I have also set -eu (probably should have added -o pipefail). I think it would be a good idea to enable strict mode and take care of initializing our variables and handling any error. Moreover, I have used shfmt to format the source code.

I have tried to follow a TDD approach. As we have already discussed, I have used bats and a couple 3rd party libraries instead of rolling our own custom test suite. To run the tests, there is script that can be called with bin/run_tests.

@nkakouros nkakouros requested a review from niieani May 20, 2019 19:26
@nkakouros nkakouros force-pushed the 3.0-named-parameters branch from 30bb771 to e832ea8 Compare May 20, 2019 21:13
@nkakouros
Copy link
Collaborator Author

I also added a travis.yml. I think I should have better reused the travis.yml from v2.

@nkakouros nkakouros force-pushed the 3.0-named-parameters branch 3 times, most recently from 3353717 to 81a92c8 Compare May 20, 2019 21:30
@nkakouros nkakouros force-pushed the 3.0-named-parameters branch from 81a92c8 to 7f216c9 Compare May 20, 2019 21:33
@nkakouros
Copy link
Collaborator Author

OK, I replaced my travis.yml with the one from v2.

Copy link
Owner

@niieani niieani left a comment

Choose a reason for hiding this comment

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

Apologies that it took so long. This is a great start! I like how simple the function is. I've added one more test.

I had some trouble running bats, due to it not being compatible with macOS's readlink, but solved that by running brew install coreutils.

We should also think a bit about the target directory structure of the v3.

It would be good to have a dependency and module system in place before we go too deep with core implementation.

@niieani niieani merged commit a81bcb0 into 3.0 Jul 21, 2019
@niieani niieani deleted the 3.0-named-parameters branch July 21, 2019 12:34
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