Skip to main content

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [Dltk-dev] debugger patches

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

Back to the top