Branching
Jesse Jones
release (and I guess ideally before the branch):
* I submitted a patch for DecorateThreadsRule which I think is a big
improvement in its behavior when the assembly doesn't reference the
thread model attribute.
* I'd like to get self-test working with DecorateThreadsRule, but
that's blocked on the above because the defect report is so poor I
don't know which assembly I need to address.
* I submitted an AddKeepAliveRule a long time ago which was reviewed
once, but a followup was never done after I improved it. Admittedly
this is for a low probability defect, but it's also a subtle one that
can cause very nasty problems so it seems like a worthy addition.
* Many of the rules have misspelled words or very awkward grammar. I'd
like to edit most of them to cleanup the language while preserving the
meaning. (At some point it would also be useful to address the meaning
as well, for example more than a few rules have iffy content, such as
example code that doesn't mark enums as serializeable).
* I'd like to change the ignore goo so that it's easier to ignore
multiple assemblies. For example, I have a project with over a dozen
assemblies. It's a serious pain to list them all in the ignore list.
It'd be way simpler to, for example, allow "*". (The rules file can
also be used for this of course, but I'd rather use one file instead
of two and I don't like using rule files because I think they will
break if gendarme adds new rule assemblies).
* There are a number of rules I have found with false positives. I've
reported a few but I don't think a single one has been fixed. Fwiw I'm
willing to fix most of them if there is a consensus that they need
fixing and the original author is busy.
-- Jesse
Sebastien Pouliot
On Jun 30, 12:21 pm, Jesse Jones <jesjo...@mindspring.com> wrote:
> There are a number of changes I'd like to see happen before the next
> release (and I guess ideally before the branch):
a) the unit tests green;
b) fix reported bug(zilla) issues (not requests);
c) bring incomplete rules;
d) pending patches;
I hope to get a+b+c done before July 20th because this will/should be
the next Novell Hack Week and I have too much things I want to hack/
prototype without feeling bad about the 2.6 backlog ;-)
> * I submitted a patch for DecorateThreadsRule which I think is a big
> improvement in its behavior when the assembly doesn't reference the
> thread model attribute.
hear feedback from anyone else using the committed rule - or better
compare it with the patch.
> * I'd like to get self-test working with DecorateThreadsRule, but
> that's blocked on the above because the defect report is so poor I
> don't know which assembly I need to address.
see how this blocks self-test since you got your own patch applied
right?
> * I submitted an AddKeepAliveRule a long time ago which was reviewed
> once, but a followup was never done after I improved it. Admittedly
> this is for a low probability defect, but it's also a subtle one that
> can cause very nasty problems so it seems like a worthy addition.
> * Many of the rules have misspelled words or very awkward grammar. I'd
> like to edit most of them to cleanup the language while preserving the
> meaning.
need for reviews in those cases. Sooner is better than later because
updating the wiki is done once per release (it's pretty time
consuming).
> (At some point it would also be useful to address the meaning
> as well, for example more than a few rules have iffy content, such as
> example code that doesn't mark enums as serializeable).
* pastebin the patches and ping the rule author on IRC; or
* send the patches to this group;
> * I'd like to change the ignore goo so that it's easier to ignore
> multiple assemblies. For example, I have a project with over a dozen
> assemblies. It's a serious pain to list them all in the ignore list.
> It'd be way simpler to, for example, allow "*". (The rules file can
> also be used for this of course, but I'd rather use one file instead
> of two and I don't like using rule files because I think they will
> break if gendarme adds new rule assemblies).
list or how "allow *" would help ignoring multiple assemblies. Please
send a use case / proposal to the list.
> * There are a number of rules I have found with false positives. I've
> reported a few but I don't think a single one has been fixed. Fwiw I'm
> willing to fix most of them if there is a consensus that they need
> fixing and the original author is busy.
positives marked as [Ignore]d. That can cut down on a lot of email
exchanges and once the problem is "seen" then patches for it are more
likely to be quickly accepted.
Sebastien
p.s. I'll be out of town (and internet connection) until next Tuesday.
Jesse Jones
On Jul 1, 2009, at 2:08 PM, Sebastien Pouliot wrote:
>
> Hello Jesse,
>
> On Jun 30, 12:21 pm, Jesse Jones <jesjo...@mindspring.com> wrote:
>> * I'd like to get self-test working with DecorateThreadsRule, but
>> that's blocked on the above because the defect report is so poor I
>> don't know which assembly I need to address.
>
> Getting self-test to work with it would be awesome. However I don't
> see how this blocks self-test since you got your own patch applied
> right?
You're correct. I'll post a patch for this.
>
>
>> * Many of the rules have misspelled words or very awkward grammar.
>> I'd
>> like to edit most of them to cleanup the language while preserving
>> the
>> meaning.
>
> Please go ahead with any fix for misspelled/grammar errors. There's no
> need for reviews in those cases. Sooner is better than later because
> updating the wiki is done once per release (it's pretty time
> consuming).
OK. I'll get started on this.
>
>
>> * I'd like to change the ignore goo so that it's easier to ignore
>> multiple assemblies. For example, I have a project with over a dozen
>> assemblies. It's a serious pain to list them all in the ignore list.
>> It'd be way simpler to, for example, allow "*". (The rules file can
>> also be used for this of course, but I'd rather use one file instead
>> of two and I don't like using rule files because I think they will
>> break if gendarme adds new rule assemblies).
>
> I'm not sure I understand the pain to list assemblies in the ignore
> list or how "allow *" would help ignoring multiple assemblies. Please
> send a use case / proposal to the list.
I have tens of entries in my ignore list that look like this:
R: Gendarme.Rules.Correctness.MethodCanBeMadeStaticRule
A: app
A: auto-complete
A: build-errors
A: continuum
A: cs-parser
A: cs-refactor
A: directory-editor
A: disassembler
A: file-system
A: find
A: make-builder
A: object-model
A: shared
A: svn
A: styler
A: text-editor
A: transcript
That's some ugly stuff and it doesn't even work that well: if I add a
new assembly then I have to update a bunch of ignore entries and if I
edit an assembly I may have to also edit the ignore list if I forgot
to add that assembly to the list. Something like:
R: Gendarme.Rules.Correctness.MethodCanBeMadeStaticRule
A: *
would be way nicer and way more future proof.
>
>
>> * There are a number of rules I have found with false positives. I've
>> reported a few but I don't think a single one has been fixed. Fwiw
>> I'm
>> willing to fix most of them if there is a consensus that they need
>> fixing and the original author is busy.
>
> I think the best way is to create a unit test that show the false
> positives marked as [Ignore]d. That can cut down on a lot of email
> exchanges and once the problem is "seen" then patches for it are more
> likely to be quickly accepted.
Well, most of these are easy to fix *if* it is agreed that it is a
real problem. So, they need discussion to figure out if the rule
really needs to be changed or if it's simply working properly but
sometimes annoying.
-- Jesse
Jesse Jones
On Jul 1, 2009, at 2:08 PM, Sebastien Pouliot wrote:
>
> Hello Jesse,
>
> On Jun 30, 12:21 pm, Jesse Jones <jesjo...@mindspring.com> wrote:
>> * Many of the rules have misspelled words or very awkward grammar.
>> I'd
>> like to edit most of them to cleanup the language while preserving
>> the
>> meaning.
>
> Please go ahead with any fix for misspelled/grammar errors. There's no
> need for reviews in those cases. Sooner is better than later because
> updating the wiki is done once per release (it's pretty time
> consuming).
I've done this for the BadPractice and Correctness rules. I should
have the rest done before you get back (except maybe Concurrency since
I have local edits there). Note that I have added TODO comments to a
few rules where the text appears to be wrong.
-- Jesse
Jesse Jones
On Jul 1, 2009, at 2:08 PM, Sebastien Pouliot wrote:
>
> Getting self-test to work with it would be awesome.
Here's a patch for self-test and DecorateThreadsRule. It:
1) Adds ThreadModelAttribute to Gendarme.Framework.
2) Marks the various report writer finalize and dispose methods as
threaded.
3) Marks a number of wizard methods and types as threaded.
4) Ignores a few wizard methods that call into gendarme from a thread.
(To fix this properly we'd have to figure out if we really do support
this and, if so, exactly which methods are thread safe. Then we'd have
to transitively annotate all those methods/types).
-- Jesse
Jesse Jones
On Jul 1, 2009, at 2:08 PM, Sebastien Pouliot wrote:
>
> Hello Jesse,
>
> On Jun 30, 12:21 pm, Jesse Jones <jesjo...@mindspring.com> wrote:
>> * Many of the rules have misspelled words or very awkward grammar.
>> I'd
>> like to edit most of them to cleanup the language while preserving
>> the
>> meaning.
>
> Please go ahead with any fix for misspelled/grammar errors. There's no
> need for reviews in those cases. Sooner is better than later because
> updating the wiki is done once per release (it's pretty time
> consuming).
I finished up at least a first cut at this. Note that I did add a lot
of TODOs. Mostly to documentation which was so unclear, or confusing,
or simply lacking that I didn't feel comfortable trying to fix it. I
also wrote new documentation for two rules because they were
documenting the wrong rule: AvoidPropertiesWithoutGetAccessorRule and
UsePluralNameInEnumFlagsRule.
-- Jesse
Sebastien Pouliot
On Wed, 2009-07-08 at 11:33 -0700, Jesse Jones wrote:
>
> On Jul 1, 2009, at 2:08 PM, Sebastien Pouliot wrote:
>
> >
> > Hello Jesse,
> >
> > On Jun 30, 12:21 pm, Jesse Jones <jesjo...@mindspring.com> wrote:
> >> * Many of the rules have misspelled words or very awkward grammar.
> >> I'd
> >> like to edit most of them to cleanup the language while preserving
> >> the
> >> meaning.
> >
> > Please go ahead with any fix for misspelled/grammar errors. There's no
> > need for reviews in those cases. Sooner is better than later because
> > updating the wiki is done once per release (it's pretty time
> > consuming).
>
> I finished up at least a first cut at this.
I have read most of them and all I can say is WOW - you did a great job!
> Note that I did add a lot
> of TODOs. Mostly to documentation which was so unclear, or confusing,
> or simply lacking that I didn't feel comfortable trying to fix it.
I think we should move them* to the wiki** so they can be
edited/updated*** from a central location and, more importantly, give us
a single link to all questions/comments/potential improvements/future
directions about a rule.
Using such sandbox should also help to avoid emails getting
lost/forgotten and help prioritize enhancements between releases.
* or at least don't add more questions, some actual TODO might be fine
** I can request/give write access to the wiki (or we could use a web
document in this google group if many people wants to update it)
*** without affecting SVN history
> I
> also wrote new documentation for two rules because they were
> documenting the wrong rule: AvoidPropertiesWithoutGetAccessorRule and
> UsePluralNameInEnumFlagsRule.
oops, looks like my review for 2.4 missed a few (but it fixed some
too ;-)
Thanks
Sebastien
Sebastien Pouliot
> On Jul 1, 2009, at 2:08 PM, Sebastien Pouliot wrote:
>
> >
> > Getting self-test to work with it would be awesome.
>
> Here's a patch for self-test and DecorateThreadsRule. It:
>
> 1) Adds ThreadModelAttribute to Gendarme.Framework.
> 2) Marks the various report writer finalize and dispose methods as
> threaded.
> 3) Marks a number of wizard methods and types as threaded.
> 4) Ignores a few wizard methods that call into gendarme from a thread.
Looks great! If it works with what's already in SVN then please commit
it immediately, otherwise I'll do my best to review your update asap.
> (To fix this properly we'd have to figure out if we really do support
> this and, if so, exactly which methods are thread safe. Then we'd have
> to transitively annotate all those methods/types).
iirc this should work/be supported if the runner is not called from more
than one thread simultaneously. The wizard model makes this a bit easier
than another GUI runner would be.
Thanks
Sebastien
Jesse Jones
On Jul 8, 2009, at 6:33 PM, Sebastien Pouliot wrote:
>
> On Wed, 2009-07-08 at 11:26 -0700, Jesse Jones wrote:
>> On Jul 1, 2009, at 2:08 PM, Sebastien Pouliot wrote:
>>
>>>
>>> Getting self-test to work with it would be awesome.
>>
>> Here's a patch for self-test and DecorateThreadsRule. It:
>>
>> 1) Adds ThreadModelAttribute to Gendarme.Framework.
>> 2) Marks the various report writer finalize and dispose methods as
>> threaded.
>> 3) Marks a number of wizard methods and types as threaded.
>> 4) Ignores a few wizard methods that call into gendarme from a
>> thread.
>
> Looks great! If it works with what's already in SVN then please commit
> it immediately, otherwise I'll do my best to review your update asap.
The tests all seem to work (except for the single failure I saw
earlier). The wizard also seems to work so I committed the patch as
r137630.
-- Jesse
Jesse Jones
On Jul 8, 2009, at 6:29 PM, Sebastien Pouliot wrote:
>
> Hello Jesse,
>
> On Wed, 2009-07-08 at 11:33 -0700, Jesse Jones wrote:
>>
>> On Jul 1, 2009, at 2:08 PM, Sebastien Pouliot wrote:
>>
>>>
>>> Hello Jesse,
>>>
>>> On Jun 30, 12:21 pm, Jesse Jones <jesjo...@mindspring.com> wrote:
>>>> * Many of the rules have misspelled words or very awkward grammar.
>>>> I'd
>>>> like to edit most of them to cleanup the language while preserving
>>>> the
>>>> meaning.
>>>
>>> Please go ahead with any fix for misspelled/grammar errors.
>>> There's no
>>> need for reviews in those cases. Sooner is better than later because
>>> updating the wiki is done once per release (it's pretty time
>>> consuming).
>>
>> I finished up at least a first cut at this.
>
> I have read most of them and all I can say is WOW - you did a great
> job!
Thanks.
>
>
>> Note that I did add a lot
>> of TODOs. Mostly to documentation which was so unclear, or confusing,
>> or simply lacking that I didn't feel comfortable trying to fix it.
>
> I think we should move them* to the wiki** so they can be
> edited/updated*** from a central location and, more importantly,
> give us
> a single link to all questions/comments/potential improvements/future
> directions about a rule.
I'd rather be more aggressive about fixing (most of) these. Confusing
or incomplete defect reports are a big deal. The defects are the
entire reason Gendarme exists after all and if users cannot understand
why a defect is reported or what the consequences of not fixing it are
then that is a serious problem.
Ideally the developers of the rules in question would edit the
descriptions and submit patches for further editing, but I expect that
would take a long time. If you like, I'd be willing to take a shot at
rewriting at least some of them and submitting patches.
-- Jesse
Sebastien Pouliot
I fully agree - which is why I think this does not belong inside the
source code.
Ideally they would all be reported into bugzilla and assigned to the
rule author (c.c. to me or the list or another list). However this could
be time consuming for the reporter (and would likely reduce feedback)
and some of the TODO are more questions / enhancements than defects.
This is why I hardly push anyone toward bugzilla (not that it would be
incorrect to use it more often). OTOH by using emails (like we do right
now) or worse source files we lose the quick "single view" of all
defects/enhancements/questions.
> Ideally the developers of the rules in question would edit the
> descriptions and submit patches for further editing, but I expect that
> would take a long time.
Patches to add/edit/remove TODO/answer/questions are not a good idea and
will reduce the value of SVN (code) history - which are against Mono
policies.
In any case both the wiki and a group document allows to see revisions,
benefit from a single view of all issues (a big plus for anyone who
needs to check many rules*) and let non-committers add their own
bugs/questions/enhancements.
* in fact this will be extremely useful for me :)
> If you like, I'd be willing to take a shot at
> rewriting at least some of them and submitting patches.
That would be great - but to avoid potential duplicated work (now and
later) I started a document [1] to track the TODO. There's no need to
remove the ones you already added (unless they are fixed, or that you
start working on them).
[1]
http://groups.google.com/group/gendarme/web/rules-todo-comments-enhancements?hl=en
Thanks
Sebastien
Jesse Jones
I'll take a shot at this then.
-- Jesse
"Andrés G. Aragoneses"
>> Ideally the developers of the rules in question would edit the
>> descriptions and submit patches for further editing, but I expect that
>> would take a long time.
>
> Patches to add/edit/remove TODO/answer/questions are not a good idea and
> will reduce the value of SVN (code) history - which are against Mono
> policies.
Sorry I didn't follow, what's the Mono policy here?
Regards,
Andrés
--
Sebastien Pouliot
The general "minimize noise around source" when committing.
Sebastien