Quoting David Roundy <[EMAIL PROTECTED]>: > On Wed, Aug 24, 2005 at 10:18:23PM -0700, Jason Dagit wrote: > > This implements the posthook option for all darcs commands and adds > > documentation as well. It does not add any test cases, I need help with > > that part. > > Thanks for the contribution! I still have a few nits to pick and a few > suggestions for where the code could be cleaner...
Okay, it's annoying from my perspective, but in the long run we all thank you for not accepting crap code :) And I do appreciate the comments on my coding style. I'm always looking for ways to improve and it helps. > If you're happier with shell, you could always implement some tests in > shell... Hmm...Shell might be easier for me, but unless I used haskell/lisp/c/c++/java I'd be learning a new language. I find perl to be really hard to grok, but shell might be a bit easier to understand. > I think this is still a possibility, but I'd definitely prefer the code to > be cleaner before accepting (and I think there are still bugs in here). It > would also be nice to throw a --unified into your send defaults, to make > reading the patch a bit easier. My main concern here is that outside of this weekend, I don't know when I'll be able to find time between now and the end of November. The problem is that my summer job is finishing up, then I move home and start my school routine again. During school I historically have very limited free time. > I don't think this does what you want it to do... either that or there's a > bug in our flag handling. This *should* mean that one can't specify both > --posthook-command and --run-posthook, since the options in a > DarcsMultipleChoiceOption are supposed to be mutually exclusive. Okay, I hadn't realized it was meant to be mutually exculsive. I have tested the way this code works (I use it on a repo with a few other contributors) it does actually work as I expected it would. But, since it violates a darcs assumption it should be changed. > I'm not terribly keen on the --prompt-posthook idea, and wouldn't mind > dropping it, in which case we'd just have two options, --no-posthook and > --posthook. It seems to me that --prompt-posthook is really only relevant > when we add support for reading (and obeying) remote defaults > configurations (e.g. the get posthook you were working on), and it would be > better wait until that is supported to add prompting support. As far as I > can tell, the default --prompt-posthook doesn't add any security, since > anyone who can modify your defaults file can as easily add --run-posthook > as --posthook. I think prompting is more inline with how darcs handles other operations. Take record, push and pull as examples. The default is to interact with the user. I don't think we lose anything here, and advanced users can put a line in ~/.darcs/defaults to never be prompted. One of the things that brought me to darcs was the level of user interaction. I feel pretty strongly about having the prompting :) If others on the list think I'm nuts they should speak up. > The other option that comes to mind would be to have two > DarcsMultipleChoiceOptions, which would be (--posthook and --no-posthook) > and (--run-posthook and --prompt-posthook)... or one could stick > --no-posthook with --run-posthook and --prompt-posthook. In that case, one > could specify multiple posthooks (and have them all run), which might make > sense. I think having prompt-posthook and run-posthook at the granularity of individual commands is best, and there is already a way to make all must posthooks run without prompting. > I haven't had time to check on how this actually behaves, so please correct > me if I'm wrong. If we keep --posthook and --no-posthook as a > DarcsMultipleChoiceOptions pair, the NoPosthook check below shouldn't be > necesary, as the two should naturally be mutually exclusive (although a > test should be added to verify this). Yes, tests are good, they bind us to functionality on corner cases. > > [added run_posthook for actually running posthooks > > Jason Dagit <[EMAIL PROTECTED]>**20050803070156 > > This adds the function run_posthook which should be used to run posthooks. > > The code was added to Test.lhs, but there may be a better place for this > > code. > > Test seems fine to me, although this could be moved directly into > DarcsCommand perhaps... I had tossed around the idea of putting some of this in a new file, Hooks.lhs or similar, but, I wasn't sure if I had enough code to justify it. I figured that would be best left to people like Ian or yourself. > You're better off with one less level of IO here, instead call get_posthook > something like Indeed, I completely forget about the extra IO level. It was useful when this code was part of Apply.lhs, but it no longer makes sense (perhaps it never did). > [snipped coding recomendations] Yes, all good ideas. > It's more than a bit ugly that there's duplicate code here. I think it'd > be a bit nicer as Heh, I was following some pseudo code you had sent me at one point, and I didn't think of the simplification that you suggest next. It's nice by the way. I shouldn't have been so literal and should have thought more for myself ;) Thanks, Jason _______________________________________________ darcs-devel mailing list [email protected] http://www.abridgegame.org/cgi-bin/mailman/listinfo/darcs-devel
