Skip to content

Conversation

@DannyBen
Copy link
Member

@DannyBen DannyBen commented Nov 12, 2021

Checklist

  • Implement ability to load YAML snippets from bashly.yml
  • Remove front matter from user files before merging them in
  • Error gracefully and informatively when the imported file is not found
    • ... or when it is not a valid YAML (for front matter)
  • Specs
  • Example
  • Docs

Key Changes

  • Added ComposeRefinements module, which applies to hashes and arrays, and imports YAML files when it encounters a hash with "import" => "path"
  • The refinement is used in the generate command, when loading the user config.
  • Front matter is supported natively by YAML::load_file.
  • Added String#remove_front_matter extension
  • The extension is used in one place only - when loading user files.

Closes #153

@DannyBen DannyBen marked this pull request as ready for review November 12, 2021 09:45
@DannyBen DannyBen requested a review from wolfgang42 November 12, 2021 09:45
@DannyBen
Copy link
Member Author

DannyBen commented Nov 12, 2021

@wolfgang42 this is your feature request - do you want to review?

The split_config example shows how to use it:

name: cli
help: Configuration splitting example
version: 0.1.0

commands:
  # Import a command that is defined in another YAML file
  - import: src/download_command.yml
  
  # Import a command that is defined in the front matter of its own shell
  # function.
  - import: src/upload_command.sh

==> hello colors
===> hello colors
Copy link
Member Author

Choose a reason for hiding this comment

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

This and a couple of others were regenerated as a side effect, ignore.


def config
@config ||= Config.new "#{Settings.source_dir}/bashly.yml"
@config ||= Config.new("#{Settings.source_dir}/bashly.yml").compose
Copy link
Member Author

Choose a reason for hiding this comment

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

This is where the composition happens, by just calling the #compose on the refined Hash, it will convert any import: path to the contents of the YAML represented by the path.


def remove_front_matter
split(/^---\s*/).last
end
Copy link
Member Author

Choose a reason for hiding this comment

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

This is called in one place only, when loading user content.

end
end
end
end
Copy link
Member Author

@DannyBen DannyBen Nov 12, 2021

Choose a reason for hiding this comment

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

This file is the "heavy lifting". It adds refined functionality to hashes, and arrays that contain arrays or hashes, to allow import: yaml_path. It fails code climate's cognitive complexity test, but I figured its ok for now.

@DannyBen
Copy link
Member Author

DannyBen commented Nov 12, 2021

Docs added here

@wolfgang42
Copy link
Collaborator

Skimmed the docs briefly and it looks very promising. I'll try it out over the weekend, but I know you prefer a faster pace of development so don't let me stop you if you'd prefer to merge sooner.

@DannyBen
Copy link
Member Author

Excellent. Its a big enough feature, I would rather have your second opinion before merging.

@wolfgang42
Copy link
Collaborator

wolfgang42 commented Nov 14, 2021

Just tried it out; it works the way I was hoping and I didn't run into any technical issues. The import system makes the bashly.yml file very easy to follow.

However, I'm not sure about the design of the front matter. Bashly understands what's going on with YAML at the top of a shell script, but nothing else does, so it seems like there's great potential for problems here. Even for a human reader it's not obvious without knowing what's going on, and syntax highlighting definitely also gets in the way. (My editor picked up on an apostrophe in a YAML string and carried that through to the end of the file, making everything the wrong colors until I adjusted the quoting to make it happy.)

I think it should probably be formatted as a comment, or maybe a specially marked HEREDOC or something, to keep it still as valid shell syntax—though I recognize that this will make it no longer plain YAML, and so will require special handling, I think that's the lesser of two evils in this case.

@DannyBen
Copy link
Member Author

DannyBen commented Nov 14, 2021

Bashly understands what's going on with YAML at the top of a shell script, but nothing else does

Nothing else needs to.... except maybe a text editor.

Even for a human reader it's not obvious without knowing what's going on

You can't say this categorically.
If people are familiar with front matter, it is crystal clear to them.
This feature is classified under advanced use. Most people are encouraged NOT to use it this way.

Front matter is a standard practice. It is the way to embed configuration in another document.

You can't complain about syntax highlighting on one hand, and then propose it to be as a comment on the other...

Personally - I think that the configuration should stay in the YAML, and the shall code in the shell script. But if someone insists on having them together, then I think a YAML front matter is the most natural and native way of doing it.

I do not want to add any new inventions and hacks in the form of comments or specially marked anything.

@wolfgang42
Copy link
Collaborator

Bashly understands what's going on with YAML at the top of a shell script, but nothing else does

Nothing else needs to.... except maybe a text editor.

Well, text editors are a pretty big “something else.” But also the author might be using shellcheck, which complains that --- is some kind of misplaced flag. Or shfmt, which will mangle the indentation of the YAML. Or they want to source the individual command scripts for a testing framework or something. Or another use case I haven't thought of entirely.

On the whole, I really think that the contents of a file named .sh ought to be parseable as a shell script, if at all possible.

Even for a human reader it's not obvious without knowing what's going on

You can't say this categorically.
If people are familiar with front matter, it is crystal clear to them.

I didn't mean to say it categorically, just that YAML is probably not the first thing most people would expect to see at the top of a file labeled as a shell script. Of course front matter is not uncommon, and I expect that most readers would figure out what they were looking at pretty quickly, but it is disconcerting.

You can't complain about syntax highlighting on one hand, and then propose it to be as a comment on the other...

Er, why not? My complaint about the syntax highlighting was that it's flat-out wrong, putting the YAML into a comment would fix that. (The YAML would still not be highlighted, of course, but at least the editor won't be trying any more.)

Personally - I think that the configuration should stay in the YAML, and the shall code in the shell script. But if someone insists on having them together, then I think a YAML front matter is the most natural and native way of doing it.

I do not want to add any new inventions and hacks in the form of comments or specially marked anything.

I can see your reasoning, and generally I think that following common standards like this would be a good thing. In this situation, though, I disagree that the front matter is most intuitive. To me it seems like more of a hack—perhaps not a hack in Bashly, but in all of the surrounding complications.

Put it this way: though I'm the one that proposed this originally, I don't think I'll use it in it's current state; I think the downsides I've discussed above outweigh the convenience of having this built in. It seems to me like it needs more thought before it's ready to be added as a feature.

@DannyBen
Copy link
Member Author

I really think that the contents of a file named .sh ought to be parseable as a shell script, if at all possible.

Front matter existence contradicts this statement. It only ever exists in foreign files.

But also the author might be using shellcheck, which complains that --- is some kind of misplaced flag. Or shfmt

Why run tools on the partials, run them on the final.

YAML is probably not the first thing most people would expect to see at the top of a file labeled as a shell script

Who is people? The only people looking at it are probably the same people who put it there....


I am currently leaning towards merging it, but I will wait for a few more days perhaps some improvement pops to mind.

@DannyBen
Copy link
Member Author

DannyBen commented Nov 15, 2021

Er, why not? My complaint about the syntax highlighting was that it's flat-out wrong

I thought you meant the fact that there is no syntax highlighting on the YAML, not that it scrambles the syntax highlighting of the shell script.

The only case where I see it scrambling the shell syntax highlighting is when you have an unclosed quote, which is super easily fixable:

hello: "User's folder"
---
echo "# this file is located in 'src/upload_command.sh'"
echo "# code for 'cli upload' goes here"
echo "# you can edit it freely and regenerate (it will not be overwritten)"
inspect_args

@DannyBen DannyBen merged commit d6f192b into master Nov 17, 2021
@DannyBen DannyBen deleted the add/yaml-compose branch November 17, 2021 06:49
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.

Allow splitting bashly.yml to smaller files (+ support front matter)

3 participants