Solution for issue#8 #11
Conversation
|
Since only some arguments are supposed to be passed to sub-make, we might want a helper method on |
Yeah, I think it's a good way to do it, I will implement the function. |
… to --help output
|
I have moved the submake arg str generation to arg.rs |
src/args.rs
Outdated
| if arg_mod.starts_with("--") { | ||
| arg_mod = arg_mod[2..].to_string(); | ||
| } else if arg_mod.starts_with('-') { | ||
| arg_mod = arg_mod[1..].to_string(); | ||
| } | ||
|
|
||
| // remove special cases | ||
| if SUBMAKE_FORBIDDEN_FLAGS.contains(&arg_mod.as_str()) { | ||
| arg_mod = "".to_string(); | ||
| } |
There was a problem hiding this comment.
Not 100% sure, but I think this would fail for an argument like -abc, where it's 3 single flags passed as a single arg. I know clap handles that, so it might be better to add this as a method under impl Args and use the struct information to construct args to pass to any sub-make.
If I'm wrong about this not working for coalesced flags, then feel free to ignore this comment.
There was a problem hiding this comment.
I will get around to this one tomorrow, should we add the args to the context?
There was a problem hiding this comment.
I don't think context should contain the args, I just think the helper should use the args struct that is attached to the Makefile.
|
I didn't get notified about your reviews, github... I will work on those :) |
|
Some, I believe unrelated tests, have started failing I will make sure they are not related tomorrow. |
|
Let me know if you want help getting this branch up to date with master; I am happy to merge master in and resolve conflicts. If you do want help, just let me know when you're at a good stopping point. |
I think I'm at a good stopping point, might want to get them merged before I work on the last point (the args one), maybe merge this and I open up another pr with the final improvement on that one? |
|
Sounds good; I'll merge master into this, fix tests, and get it merged and we can continue work on this is another branch. |
This is my initial solution, needs the tests and such, but I wanted to get your opinion on it before going further in this path.
Should fix #8