Skip to content

Conversation

@SidIcarus
Copy link

@SidIcarus SidIcarus commented Sep 5, 2019

Note

I kept cmd|command separate from available|installed for two reasons:

  • to not break backward compatibility
  • because really determining if a command is available, installed, or is just
    command, is more complicated; see has

I intended for these changes to be separate PR's but I got caught up and it got
a little too tedious to re-structure the commits.

Since this doesn't break backwards compatibility, I've updated the version from 1.1.2 -> 1.2.0

Changes

  • features

    • options:
      • no args will display --version followed by --help
    • conditions
      • alias, builtin, keyword, function|fn, int|integer,
        array, hash|dictionary, export|exported
      • bool|boolean, truthy, falsey
      • cmd|command
      • in
      • set|var|variable
    • .editorconfig, .gitignore
    • test
      • can now be passed in multiple arguments to test against the same condition
  • maintainability

    • version & help are in their own function
      • printf over heredoc + cat
    • reused logic for several conditions
    • printf over echo
      • echo doesn't consistently behave the same across all systems
    • use builtin/internal functionality from bash over external tools
      • since this tool doesn't check dependencies, it'll be safer
    • removed duplicate codes
  • style

    • spacing: 4 -> 2
    • printf over heredoc to not have tabs or un-indented code
    • assert.sh: move code continuation so syntax highlighting in VSCode doesn't
      mis-color the rest of the file

In the tests file, I kept reference to a bash bug I encountered when parsing the keyword & builtin, [[]] & [], respectively. I can add a note about what version this was found in.

- removes the need to do it in either path
  future commit can further remove the need to set the subsequent vars
- increases maintainability & testability
- `echo` is not standardized across platforms
- increases maintainability and allows for clearer review of history
- removes usage of external tools `cat`, as `printf` is a builtin
- easier to adhere to style since we no longer have tabs & spaces
- reduces duplicate hardcoded strings
…ion|fn`

- excludes `_type` option from help so its functionality is not
  misconstrued for working for more types
- allow status code to propagate down to end
- increases maintainability and clarity
- added post & pre-warming logs to indicate the beginning
  and completion of the warming
- since `assert` begins counting as soon as is sourced, it is
   misleading to have it be sourced before we warm
@SidIcarus
Copy link
Author

@qzb
can you take a look?

@qzb
Copy link
Owner

qzb commented Sep 11, 2019

Thanks for your PR, I'll try to review it today or tomorrow, but for now it looks great 😀

* `is function NAME` - name is a function
* alternate(s) `fn`
* `is keyword NAME` - name is a keyword
* `is array NAME` - name is a variable
Copy link
Owner

Choose a reason for hiding this comment

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

I guess it's kind of obvious, but still description should be more precise:

Suggested change
* `is array NAME` - name is a variable
* `is array NAME` - name is a variable of array type

Copy link
Author

Choose a reason for hiding this comment

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

If the intent of the README condition description is to tell you what you need to pass in to get a true resultant back, then yes. Would you want to update the rest of the type conditions to follow suite?

if is equal $var 123.0; then
echo "it just works"
fi
$ is equal "$var" 123.0 && printf 'it just works!\n'
Copy link
Owner

Choose a reason for hiding this comment

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

Why use printf in the examples? Isn't echo more common and easier to use (no \n at the end)?

Copy link
Author

Choose a reason for hiding this comment

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

from what I recall, printf is more portable & consistent then echo across platforms

Copy link
Owner

@qzb qzb left a comment

Choose a reason for hiding this comment

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

Some of new conditions work only when script is sourced. I think it should be described in README.md and help messages. Personally I would move all this conditions to separate section in README.

- `bash v3.2.57` doesn't support `declare -g`
- internal lowercasing is faster then `tr` so the minor speed reduction
  due to the conditional is worth it overall
- unfortunately, this relatively increased the test duration quite a bit
  0-1s to 2-3s on v5 & 4-5s on v3.2.57
- to keep with backwards compatibility
  previously, you could write `not an not an the not a`
  but with the change to using a while loop for the articles whilst
  keeping its check after not, you lost the ability to do that
otherwise, the timing is not as percise
@SidIcarus
Copy link
Author

Some of new conditions work only when script is sourced. I think it should be described in README.md and help messages. Personally I would move all this conditions to separate section in README.

Good idea. I'll fix up the tests to reflect this as well.

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