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

WIP: Batch #2 #5

Closed
wants to merge 17 commits into from
Closed

WIP: Batch #2 #5

wants to merge 17 commits into from

Conversation

zph
Copy link
Contributor

@zph zph commented Mar 18, 2021

WIP & TBD.

Context in #3 and #4 which contains Batch #1 of changes.

This PR are proposed changes subject to revision as we refine the good ergonomic concepts into what will merge and ship πŸ—‘οΈ .

Current CLI output for tome --help
image

Each subcommand has its own targeted help: tome exec --help
image

Then running a subcommand: tome commands --directory ./example
image

List of changes:

  • TBD

TODO for merge:

@toumorokoshi
Copy link
Owner

Looks like this also includes changes from #4. How about a review after #4 is merged?

@toumorokoshi toumorokoshi self-requested a review April 4, 2021 21:15
@toumorokoshi
Copy link
Owner

@zph it looks like there's some conflicts that keep the diff from showing vs the newly-merged code, so I can a code review once that's merged in.

Regarding the UX of the CLI itself: Here are my high level design considerations and thoughts:

  • huge +1 to having some argparse library, to enable better help and CLI for the tome command itself.
  • are there any backwards-incompatible changes from the previous version of tome?
  • are there any backwards-incompatible changes from sub?
  • what sort of commands will be provided to the tome instance, vs the tome cli itself. e.g. {your-command} --help is automatically provided, as is {your command} help {foo} since I believe sub provided the same functionality.

Given these considerations, I'm wondering if we should perhaps start with a design.md-style file to start to align on these philosophical choices and strategies? If you agree, I can author one with some thoughts.

@zph
Copy link
Contributor Author

zph commented Apr 4, 2021

A design doc together sounds great. I can reply and fill in details there later today πŸ‘

Copy link
Contributor Author

@zph zph left a comment

Choose a reason for hiding this comment

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

@toumorokoshi I fixed the merge conflicts but I've lost the front of mind context of these changes and this is a big PR.

We will work through the design doc, then maybe I can split out this PR into pieces to shrink the merge size.

@@ -6,9 +6,9 @@ edition = "2018"
license = "MIT"

[dependencies]
tui = "0.3.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed because they didn't look used.

termion = "*"


clap = "3.0.0-beta.2"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chose beta because of most up to date auto-generated shell completions.

Copy link
Owner

Choose a reason for hiding this comment

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

I think it's fine, ultimately we produce a compiled binary so the use of beta internally is less of an issue I think.


set -eou pipefail

cargo test --verbose
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Convenience scripts, we can skip if you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reasoning for them: never needing to wonder how to do an action in a given project when moving between projects or when new people contribute.

Copy link
Owner

Choose a reason for hiding this comment

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

I don't have a strong opinion. I've seen Makefiles used for this purpose as well, but we can always clean up if things get unwieldy.


macro_rules! help_template {
() => {
r#"echo -e
'This is an instance of tome, running against the directory {}.
\nThe commands are namespaced by the directory structure.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I simplified the escaping by printing this directly via eprintln vs trying to echo it.

}
}
};
let shell_env = env::var("SHELL").unwrap();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: move away entirely from trying to infer shell. Ask user to supply it :)

consuming_help = false;
} else if let Some(rest) = line.strip_prefix("# ") {
} else if line.contains("# ") || line.contains("// ") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: generalize this to other common prefixes?

Copy link
Owner

Choose a reason for hiding this comment

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

are you suggesting we should support other comment prefixes (e.g. '!' or ';') as well?

I think that seems reasonable. I would prefer to not get into the business of capturing multi-line comments though, since it forces us to write some level of recursive parsing or escaping.

@@ -113,7 +121,7 @@ impl Script {
CommandType::Execute => {
let command_string = if self.should_source {
// when sourcing, just return the full body.
let mut command = vec![String::from("."), self.path.clone()];
let mut command = vec![String::from("source"), self.path.clone()];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reading comprehension, source should be anbalagous to . but more self explanatory.

Copy link
Owner

Choose a reason for hiding this comment

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

source is non-posix, so we should stick with dot. See the posix spec.

Copy link
Owner

Choose a reason for hiding this comment

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

of course zsh and fish are not completely posix compliant, but I think we should strive to use posix semantics unless we have a strong reason not to.

let mut escaped_command_string = vec![];
for mut arg in command_string {
arg = arg.replace("'", "\\'");
arg.insert(0, '\'');
arg.push('\'');
escaped_command_string.push(arg);
}
// Include commandline arguments
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were left out before.

@@ -42,7 +42,7 @@ fn test_help() {
",
)) as Box<dyn Read>,
);
assert_eq!(&script.help_string, "foo bar baz\n");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

πŸ€” should we have that newline or no?

toumorokoshi added a commit that referenced this pull request Apr 6, 2021
Authoring a design goals document, to help document agreements and rationale for the design of the CLI interface.

This will help further other PRs, including #5.

Includes the following PRs:

* Incorporate PR review discussion into design doc (#14)

Co-authored-by: Zander Hill <zander@xargs.io>
@toumorokoshi
Copy link
Owner

thanks for the changes! I'll take a closer look at the code in the next couple days, especially now that #13 is in.

@zph
Copy link
Contributor Author

zph commented Apr 6, 2021

@toumorokoshi Perfect. I'll tag you here when I get a chance to get back to it... likely later this week or during weekend πŸ˜„. Until then no need to spend time reviewing as it may undergo moderate change to match our spec in #13 .

@zph
Copy link
Contributor Author

zph commented Apr 13, 2021

Update, I didn't get to work on it this weekend but will as schedule allows πŸ€—

@toumorokoshi
Copy link
Owner

toumorokoshi commented Apr 14, 2021 via email

@toumorokoshi
Copy link
Owner

@zph happy new year! My cycles are starting to free up, so I think I want to take the step to try to get tome to 1.0 and start trying to get some buzz on it.

Is it alright with you if I move forward with the design that we discussed? If you still have the cycles to contribute you're more than welcome, but don't want to burden you if you have other things going on.

@zph
Copy link
Contributor Author

zph commented Jan 3, 2022

@toumorokoshi I'm excited to hear that you're freeing up for more Tome! I'm in strong support of you carrying on with the design we discussed :D.

I can't offer to do much on it myself right now, but I could definitely exchange a few messages or look over code in a PR if you want me involved.

Happy New Year!

PS - I know a friend who's using this for their commandline tooling and can see if they're free to also try out an RC once it's near v1.0.

@toumorokoshi
Copy link
Owner

@toumorokoshi I'm excited to hear that you're freeing up for more Tome! I'm in strong support of you carrying on with the design we discussed :D.

I can't offer to do much on it myself right now, but I could definitely exchange a few messages or look over code in a PR if you want me involved.

Happy New Year!

PS - I know a friend who's using this for their commandline tooling and can see if they're free to also try out an RC once it's near v1.0.

Sounds great and thanks for all of your help! I'll send some pings once I scope the 1.0 milestone.

@Nattle
Copy link

Nattle commented Jan 4, 2022

Just wanted to pop in and say that I use tome all the time. I have 3 separate commands that I use for work stuff in different scopes and another tome that I used for my personal stuff.

I appreciate all the work!

@toumorokoshi
Copy link
Owner

Hello! I think all of the changes that would have been introduced in this PR have been incorporated in the main branch (use of clap, new reserved commands, fish support).

With that, I think we can close this PR out. Thanks for all the help!

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.

None yet

3 participants