Re: [Dltk-dev] debugger patches

[Dmitriy Kovalev <kds@xxxxxxxxx>]

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 just synced against the HEAD to pull down the commits that contained 
> the patches that i submitted, and i am curious to know why some of the 
> changes that i made did not make it. i'd like to try and improve the 
> work submitted in the future, and also discuss why i may disagree with 
> some of the reverts.

Please discuss, it's not a problem
> - 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.

> - 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.
> - 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.
> - 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.

> 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.

--
Dmitriy Kovalev