WIP: get rig of pub doc(hidden)

[CreepySkeleton]

That is my attempt of removing the pub #[doc(hidden)] blasphemy. It is not ready yet - far from being ready, in fact. I'm just publishing it to get some feedback from @TeXitoi and @pksunkara .

[pksunkara]

You are mixing so many things here. I like some parts, some parts I want to discuss. I especially dislike the view parts. Can you separate them into different kinds of PRs?

[CreepySkeleton]

The view parts are the core point, they serve as interpolation API. Everything else is build on top of them. Could you be more specific about want you like and what you don't?

[pksunkara]

Basically, there are few things not related view but still related doc(hidden) here. Let's separate them out and get them merged. Then, we can discuss about view.

[CreepySkeleton]

Most of those things are ought to be pub because they are needed by clap_derive. I already told you that the crates are heavily intermixed to the point they are essentially the same crate.

[pksunkara]

The views are too much work and complexity for macros that are public just because they are needed in clap_generate. That is basically my opinion on this PR. Everything else looks good to me.

[CreepySkeleton]

So, you think this huge amount of pub hidden is just because it's used in macros? If so, you're only partially right; in fact, to a very little extent.

• Not exactly related here, but just an example
  

  clap/clap_generate/src/lib.rs

  Lines 183 to 185 in 2370665

  	if !app.is_set(clap::AppSettings::Built) {
  	app._build();
  	app._build_bin_names();

• sc.about

  clap/clap_generate/src/generators/shells/zsh.rs

  Lines 148 to 160 in bc738e1

  	let s = format!(
  	"\"{name}:{help}\" \\",
  	name = n,
  	help = sc
  	.about
  	.unwrap_or("")
  	.replace("[", "\\[")
  	.replace("]", "\\]")
  	);
  	if !s.is_empty() {
  	ret.push(s);
  	}

• sc.name

  clap/clap_generate/src/generators/shells/zsh.rs

  Line 167 in bc738e1

  add_sc(sc, &sc.name, &mut ret);

• p.has_subcommands() (hidden fn)

  clap/clap_generate/src/generators/shells/zsh.rs

  Line 212 in bc738e1

  p.has_subcommands()

  
  

  clap/clap_generate/src/generators/shells/zsh.rs

  Line 297 in bc738e1

  let sc_or_a = if p.has_subcommands() {

• p.bin_name (note: there's the public, well documented get_bin_name method)

  clap/clap_generate/src/generators/shells/zsh.rs

  Line 300 in bc738e1

  name = p.bin_name.as_ref().unwrap().replace(" ", "__")

Those are just the usages my eye picked upon a shallow glance, there are many more of those to go on apart from macros. If ou are really curious, quickly change App and Arg fields frompub to pub(crate), run cargo check, and you'll see the devil 😈.

My point is, you're underestimating how deeply these two crates are tangled together. It's not "just macros".

[pksunkara]

I have given it a bit of thought. I understand that we need the views. But can we try to minimize the fields inside them so that we are only doing for the fields we need?

[CreepySkeleton]

minimize the fields inside them

Of course. As I stated, the work is a deep-in-progress. I've just copied the fields from App/Arg because the "everything at once" was simpler than figuring which specific fields I would need. The plan is to remove any excessive fields once it's done.

[CreepySkeleton]

Also, what do you think about #810 (comment)?
That would be alternative to this PR.

[pksunkara]

I would much prefer this PR than doing that. The reason we want clap_generate separate is that we want to give tools to the end users to write their own generators for their own weird things.

[CreepySkeleton]

That issue would be than unsolvable due to circular dependency. 10.03.2020, 19:26, "Pavan Kumar Sunkara" <notifications@github.com>:

…

I would much prefer this PR than doing that. The reason we want clap_generate separate is that we want to give tools to the end users to write their own generators for their own weird things. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[pksunkara]

We should be able to solve that issue with macros. If they want those commands in their apps, they can use the macros. Or we can even provide a wrapper function in clap_generate. I think we can solve it without moving the generators into clap.

[pksunkara]

Note for anyone looking, this needs a rebase on master since the above linked PR was created out of parts of this.

[pksunkara]

What about creating `AppBuilder` and `ArgBuilder` structs to do the builder pattern properly? The only downside of this all the chaining of args and apps need to end with `.build()`.

[CreepySkeleton]

What about creating AppBuilder and ArgBuilder structs to do the builder
pattern properly? The only downside of this all the chaining of args and
apps need to end with .build().

App and Arg are builders already. Are you suggesting to introduce a builder that builds a builder? [Yo dawg joke]

[CreepySkeleton]

I'm positive that this was a spurious failure. My local test passes just fine and I haven't touched any code that could possible affect this. I'm flushing travis cache and restarting the build.

[CreepySkeleton]

@pksunkara Do you want me to squash the commits into one?

[CreepySkeleton]

Failure due to tarpalin is being unable to wait until ui tests have passed. I propose to exclude clap_derive from coverage, they don't count anyway.

[pksunkara]

Yes, please squash all the commits. Just have 2 minor comments. Once you fix them, I will merge this PR and you can create a new PR for the other doc(hidden) things.

[pksunkara]

Should we add app.get_positionals()? This exact logic is being repeated in macro positionals! also. Would be nice to keep it DRY.

[CreepySkeleton]

get_positional borrows the whole app immutably, but we do app.settings.set() several lines below (mutable borrow.

[pksunkara]

bors r+

[bors]

Build failed

• continuous-integration/travis-ci/push

[pksunkara]

bors r+

[bors]

Build succeeded

• clap-rs.clap
• continuous-integration/travis-ci/push
• Release Profile Tests