thanks for your response, my comments are in-lined below.
On 7/13/07, Dmitriy Kovalev <kds@xxxxxxxxx
> wrote:Hello Jae,
I want to clear up the situation about debug patches. I'm continuously
working on improving DLTK debug framework and especially ruby debugging
support. Therefore I always have some changes that's not commited to the
HEAD. I can't directly apply your patches in my code and I'm reluctant
to make this procedure by hand. Probably I missed some documentation
comments… so I'll check your patches again.
i completely understand. i'm sure what makes this even harder is that i'm off working in my own little world trying to get my plugin implementation going, so as i find things that need refactoring, etc i'm potentially stepping over something else that is already in progress. perhaps i should drop a line to the list when i'm working on a particular area to see if any other work is going on.
> - i wrote javadoc and left @see comments on methods that were
> overridden, etc. those have been removed. this one surprises me the
> most b/c i've seen people asking about documentation and i was trying
> to fill that gap when/where i could. (i find the @see comments useful
> b/c i know exactly what method is being overridden/implemented in the
> class hierarchy). it seems that some documentation is better then no
> documentation, so if there are guidelines on this, i'd like to know.
I'll check your patches again for missed documentation. There are no any
guidelines now but we'll discuss it in near future.
for the most part it was a bunch of @see comments that got dropped and some other minor javadocs (for instance, in the DLTKUILanguageInterface, i put some javadocs about what the getPreferencePageId(), etc did - somewhat self-explanatory, but still - i noticed those didn't make their way in).
as i said before, i find the @see comments useful to know exactly what class is method is being overridden/implemented from. if you don't want to use them in the source, i'll be more then happy to not include that w/ the patches. as for the javadocs, i'll keep submitting those where i can and just complain if they don't show up in the commit. :)
> - the calls to save the plugin preferences were moved to the
> AbstractScriptDebuggingEngineConfigurationBlock b/c that's what the
> IPreferenceConfigurationBlock interface states its performOk method is
> supposed to do, so i felt this change brought things more in line w/
> the api - is this not the case?
Please, give additional comments about
AbstractScriptDebuggingEngineConfigurationBlock because I can't find any
problems in the current code.
there isn't a problem in the code - everything works correctly, i just felt that based upon the api documentation for the PreferencePage class (which the AbstractBlockConfigurationClass extends) and the IPreferenceConfigurationBlock class (which the ImprovedAbstractConfigurationBlock extends, which is then extended by the ExternalDebuggingEngineConfigurationBlock), the logic place to actually invoke the call to save the preferences actually belonged inside the concrete implementation of the IPreferenceConfigurationBlock.
maybe this is just a severe nitpick on my part b/c invoking the call to save the preferences in the PreferencePage subclass - i was just trying to hold true to what the api documentation states.
> - methods were added to the InterpreterConfig class to encapsulate
> getting/setting the host, port, override_exe etc so that that classes
> that interact with those common properties did not need to know what
> keys were used to store them - they could just call set/get methods to
> adjust them accordingly. the manner currently being used breaks the
> idea of encapsulation and doesn't insulate the sub-classes from
> changes that may need to be made that could affect those common
> properties.
I disagree with you about InterpreterConfig changes because this is a
general class that used for launching purposes across all DLTK. It
should not depend on DbgpConstants class that's specific only for
debugging. My suggestion is to make additional helper class (that gives
simple access to InterpreterConfig class) only for debugging purposes. I
'm thinking about simple and useful solution and appreciate any help.
that is a fair argument. it seems that be both agree that it doesn't make sense for the sub-classes to have to know how to retrieve the property values - now it's just a matter of where to put the implementation that helps hide it.
perhaps a DebugInterpreterConfig sub-class, or just a helper class that has some static methods on it that take the InterpreterConfig object as an argument and returns whatever value you're looking for.
ie:
public static String getHost(InterpreterConfig config) { ... }
i am working on getting remote debugging going (i already have a basic implementation that works w/ some hard coded values - the interface will look more or less like the remote java debugging tab, minus the drop down selector) which is pretty much a generic thing for all dltk languages b/c only the dbgp server needs to be launched, not an actual process. the launch configuration is going to have to support defining the ide key that will be used to connect, as well as potentially defining a different listener port.
> - it looks like the work i did for the debugging path validator got
> moved around, but not the error messages (i tried to clean up the
> grammar a bit more, no offense meant by this).
>
Path validator now located in FieldValidators class. I've used your idea
about field validation and also created new experimental class
ImprovedAbstractConfigurationBlock as a replacement for
AbstractConfigurationBlock.
i saw that - a much better implementation that i had come up with. my issue here was that the error messages for the validator didn't make it in. i just cleaned up the grammar (again, no offense) in the existing messages. this is not that big a deal, i'm more then happy to resubmit a new patch that cleans it up again.
> great work on everything overall, i'm glad i was able to make the
> contribution - hopefully they will be a real benefit to others.
>
Thank you, as Andrey said we are also excited about your contribution!
The work of merging our code with your patches is still in progress.
overall i just wanted to understand why some changes were made, some not, etc. i completely understand things being missed as well - those were some pretty hefty patches, and i'm sure it was no small effort to merge them in with existing changes that you already had. i'm glad to be helping out and excited that i'm having an impact on the overall project, and am looking forward to making further contributions.
