Closed GoogleCodeExporter closed 9 years ago
Original comment by jbev...@gmail.com
on 21 Nov 2007 at 10:17
[deleted comment]
Hi there!
What exactly do you expect in this task? Is it also about changing the
interface?
Original comment by benedikt...@gmail.com
on 1 Dec 2007 at 11:10
Attachments:
Wrong URL, it should be: http://www.mono-project.com/Gendarme
Another useful one is: http://groups.google.com/group/gendarme
Original comment by sebastie...@gmail.com
on 4 Dec 2007 at 1:20
I claim this task.
Original comment by dan.abra...@gmail.com
on 6 Dec 2007 at 3:53
Original comment by jpo...@gmail.com
on 6 Dec 2007 at 4:03
Can you please give me some additional time for that task?
Unfortunately it happened so it's only now that I have time to begin working on
it.
And, by the way, what is expected amount of work?
I'm now working on implementing some of those naming rules:
http://www.gotdotnet.com/Team/FxCop/Docs/Rules/Naming.html
Original comment by dan.abra...@gmail.com
on 8 Dec 2007 at 10:02
The expected delivery is one naming rule, for a week long work. To be accepted,
a
rule has to come with its corresponding tests, and the patch ready to be
integrated.
Also, you should check that Gendarme doesn't already have a similar rule.
Original comment by jbev...@gmail.com
on 10 Dec 2007 at 2:19
Huh, it's okay then, as I have already implemented four :-)
Original comment by dan.abra...@gmail.com
on 10 Dec 2007 at 2:42
Somewhy I couldn't manage to get a working svn diff so I just attach "new"
Gendarme.Rules.Naming directory.
To make it work, checkout the latest sources, delete _everything_ in
rules/Gendarme.Rules.Naming directory and extract tarball's contents into it.
Hope that'll work.
Original comment by dan.abra...@gmail.com
on 10 Dec 2007 at 10:01
Attachments:
Will anyone check that out?
Original comment by dan.abra...@gmail.com
on 12 Dec 2007 at 7:31
Of course we will.
Thing is that we sometimes tend to be busy you know?
I can't review the code if it's no in unified diff (svn diff).
Moreover, the code as it is mixes spaces and tabs, so it's:
1) hard to review
2) unmergable with gendarme.
Original comment by jbev...@gmail.com
on 12 Dec 2007 at 8:18
Hello Dan,
Could you explain the problem you had to produce a diff using SVN ?
That would ease the review quite a bit.
Thanks!
Original comment by sracque...@gmail.com
on 12 Dec 2007 at 9:13
Thank you very much for your comments.
Tabs and spaces mix must be my MonoDevelop settings problem. I'll be more
careful
next time. However isn't this just a matter of replacing all ' ' with
'\t'?
Anyways, I'll fix it this evening.
And then I will try svn diff it once again.
Thanks!
Original comment by dan.abra...@gmail.com
on 13 Dec 2007 at 5:25
Dan: Tabs-to-spaces can be a lossy conversion. For example, my preferred style
is to
use tabs for indenting scopes, and spaces for aligning things past the
indentation
level of the scope. That way you can change the displayed tab width without
visually
messing up code alignment. This is the style that MonoDevelop's "smart"
indednter
uses right now. Hitting the tab key will reindent the current line how the
indenter
thinks it should be.
Of course, if a project uses only spaces or only tabs, the conversion isn't a
problem. I believe Mono's policy in general is pure tabs, though my mixed style
fits
in fine. Note that "8-space tabs" means actual \t tabs, but displayed with a
width of
8 spaces (this is the default tab width in pretty much every terminal, so I
don't
know why code editors tend to default to 4).
Original comment by m.j.hutc...@gmail.com
on 13 Dec 2007 at 5:41
I attach the diff, however still I couldn't manage to patch a fresh local copy
with
it (I tried `patch -p0 < path/to/my/diff`).
P.S. Just build the Gendarme.Rules.Naming and its test project, as other ones
don't
want to compile against fresher Mono.Cecil.
Original comment by dan.abra...@gmail.com
on 13 Dec 2007 at 7:39
Attachments:
I won't have time to test it until this weekend but the diff itself looks great
:-)
One thing you may want to do is update your Mono.Cecil (the one used by the
current
source code is newer, not older, than your own). Also, at this moment, the main
build
mechanism are the Makefile (if you want to try your hands at them ;-). This may
explains why you had some problems compiling inside MonoDevelop (which, last
time I
used it, was still using an old Mono.Cecil.dll).
Original comment by sebastie...@gmail.com
on 13 Dec 2007 at 8:25
JB or Sebastien, could you review and close this? Students can only do one task
at a
time, so we're preventing Dan from claiming something else.
Original comment by m.j.hutc...@gmail.com
on 19 Dec 2007 at 5:17
Dan, you can claim another issue if you want while we review this one.
Original comment by m.j.hutc...@gmail.com
on 19 Dec 2007 at 6:11
Dan,
UseCorrectPrefixRule and DoNotUseIncorrectPrefixRule should be merged into a
single
rule (with a specific message in each case). Can you attach the merged files
(rule
and test) in an archive (not a patch, since I'm having bug trouble with the
existing
one and those will be new file).
Except for this merging in Gendarme is mostly done (the string changes are
already in
SVN) and all I'll start to test (outside the unit tests) the rules.
Thanks (and sorry for the delay).
Original comment by sebastie...@gmail.com
on 21 Dec 2007 at 3:40
Ok, self-test is turning a lot of false-positive.
The naming rules needs to check for compiler generated code and other stuff out
of
"user" control.
e.g. the check for pascal-cased [type|method] names must ignore things like
<Module> type
.ctor and .cctor methods (constructor and static constructors)
get_* and set_* methods (properties getters and setters)
Original comment by sebastie...@gmail.com
on 21 Dec 2007 at 4:28
[deleted comment]
Will be fixed soon.
Here's merged UseCorrectPrefix stuff and updated .xml.in file.
Original comment by dan.abra...@gmail.com
on 21 Dec 2007 at 5:16
Attachments:
Great :) this is now merged (in my local tree)
self-check (gendarme running on gendarme) reports
222 defects for UseCorrectCasingRule: 175 defects, mostly (or all) false
positives;
3 defects for UseCorrectPrefixRule: all valid
1 defects for UseCorrectSuffixRule: valid
0 defects for UsePreferredTermsRule
Other existing rules executed on yours results in [1]
UseCorrectPrefixRule [2]
1x MethodCanBeMadeStaticRule [3] (for HasPrefix)
UseCorrectCasingRule [4]
4x MethodCanBeMadeStaticRule [3] (for IsPascalCase, PascalCase, IsCamelCase,
CamelCase)
So good job!
[1] excluding UseCorrectPrefix results
[2] I'll fix this one before committing (since it's ready for commit in my tree)
[3] it's a new rule and we have a lot of defect wrt it.
[4] since you have to pending fix in this one, you can "static-ify" the methods
p.s. in case you're interested a new Gendarme task has been added to the
current list
(see #59).
Original comment by sebastie...@gmail.com
on 21 Dec 2007 at 7:41
Well, I've done some work so
[*] Now it all compiles against Mono.Cecil 0.6
[*] I hope now no false positives
However, it seems to me that there are some 'false negatives'. I had been
running
gendarme over the same assembly and 've been having lots of _true_ violations -
and
then something happened and now I have _lot_ less of ones. However I could not
find a
source of this bug (if it is a bug).
Hope you'll help me find out what's gone wrong (test are running okay, though).
P.S. I attach the whole folder so you could just copy everything to your folder
and
merge it. However I'd advice you to back your copy up because my code must have
a
problem described above.
P.P.S. Sorry for my poor English and delays :-)
Original comment by dan.abra...@gmail.com
on 22 Dec 2007 at 10:38
Attachments:
Dan, unless you tell me you fixed something specific in some rule, I'm not gonna
update everything (since it's all merged and tested in my tree). So I'll only
update
(and test) your new UseCorrectCasingRule.
From a previous *quick* view of the 200-ish false positives in self-test I
haven't
seen one valid instance. However I won't take chance and will review all of them
before updating UseCorrectCasingRule. Thanks for the heads up!
Original comment by sebastie...@gmail.com
on 22 Dec 2007 at 11:13
I check the previous results and didn't found any valid case (in self-test).
The only
case I've seen are:
<Module>
<PrivateImplementationDetails>
.ctor
.cctor
get_*
set_*
anonymous methods
All valid cases since they are out of control of the developer.
Now your archive seems to be missing the UseCorrectCasingRule.cs file, the unit
tests
are still there and so is a new NamingUtils.cs file. Could you attach only the
needed
file(s) ?
Original comment by sebastie...@gmail.com
on 22 Dec 2007 at 11:37
[deleted comment]
Hmm..
Do you have your working copy commited in any branch or smth? I need it as a
base to
start applying new changes.
If not, please, create a Google Code project and commit there your local copy
(I mean
one you mentioned in #24). I need it ready to compile so if your Mono.Cecil is
older/newer than 0.6, post it there too please.
Thanks in advance.
Original comment by dan.abra...@gmail.com
on 23 Dec 2007 at 11:20
Oh - and here UseCorrectCasingRule.cs is:
Original comment by dan.abra...@gmail.com
on 23 Dec 2007 at 11:22
Attachments:
Thanks for the missing file.
It seems that you took the hard road (maybe I should have hinted you) since the
same
problem was already fixed in existing rules.
In details
* NamingUtils methods shouldn't be checking for $ or < in strings. There are
better,
and safer, ways to do this (look at AvoidUninstantiedInternalClassesRule);
* NamingUtils has a bit of duplication (e.g. checking for
CompilerGeneratedAttribute)
and Gendarme will report this (the smells rules);
* UseCorrectCasing now totally ignores get_ and set_ while it should not. The
rest of
the name still needs casing validation;
* The same applies for add_ remove_ and other special names (look at
Mono.Cecil.MethodSemanticsAttributes). The prefix should be skipped, but the
rest of
the name should be validated;
* New unit tests should be added to catch/test those cases.
Thanks
[note: I'm only the reviewer here. I will provide a patch, in another comment,
but I
won't create a new repository for your task]
Original comment by sebastie...@gmail.com
on 23 Dec 2007 at 3:53
Here the diff of my gendarme/rules/Gendarme.Rules.Naming directory
Original comment by sebastie...@gmail.com
on 23 Dec 2007 at 3:57
Attachments:
What I get from gendarme SVN does _not_ compile against the freshest Cecil
(0.6, as
mentioned in mono docs), and before applying the diff I'd like it _to_ compile
:-) .
I have maid some changes to code (e.g. replace deprecated
TypeDefinition.IsPublic
calls etc) and posted them in #25 but, as everything seems to compile on your
machine, I ask you again: what version of Mono.Cecil do you use? Can you just
post it
here or send me the file itself by e-mail?
Of course I cannot make changed to code unless it compiles correctly so hope
you'll
help me to handle this.
Thanks for patience,
Dan.
Original comment by dan.abra...@gmail.com
on 23 Dec 2007 at 8:16
Gendarme from SVN always compile against Cecil from SVN.
Depending on how you checked out Gendarme you probably already have the latest
Cecil
sources around (or else it's easy to get it). Inside /cecil/lib you'll find a
standalone makefile (standalone.make iirc). You can build and install the most
recent
Cecil using this makefile.
make -f standalone.make
make -f standalone.make install
Then you should be able compile the latest Gendarme without problem.
Another option, which I have not tried, is to use MonoDevelop to open the VS.NET
solution file that Cecil provides (I believe it's current).
As a last resort continue with your current Cecil(*) and concentrate on fixing
the
UseCorrectCase rule (with tests). This is the single missing thing to close
this task
(but obviously I'm willing to help you if you want to fix other issues related
to
Gendarme/Cecil :-)
(*) I'll make the required (and small) changes before the commit to SVN
Original comment by sebastie...@gmail.com
on 23 Dec 2007 at 10:20
[deleted comment]
I've compiled everything as you said (at last).
Some rules in Performance assembly fail, by the way, as seen in the attached
screenshot, However that should not worry me, should it? :-)
Now I'm working on naming rules, hope I'll be able to finish it all by the
evening
(about 19.00 UTC).
Original comment by dan.abra...@gmail.com
on 24 Dec 2007 at 11:59
Attachments:
No it shouldn't worry you ;-)
MonoDevelop use an older version of Mono.Cecil, so some symbols are missing.
Right
now the "supported" way to build/test Gendarme are the Makefiles (i.e.
MonoDevelop
and VS solution/projects files are supplied but user-supported).
Note that this will probably change in the future, since we will
(a) want the MD addin to work properly;
(b) probably (pending tests) use MD to export the Makefile and VS.NET solutions;
Original comment by sebastie...@gmail.com
on 24 Dec 2007 at 3:28
Hola!
At last I managed to make it all work as intentioned!
:P
Note that I still check if type name contains some characters ($, < and >)
because
unfortunately it doesn't work the other way (see lines commented out in
NamingUtils.cs).
Kinda changelog:
[*] UseCorrectPrefixRule.cs
Test/UseCorrectPrefixTest.cs
-- now doesn't take 'CLS' or other abbreviations as rule violations (e.g.
'CFG' is not like 'CMyCLass' + corresponding test cases
[*] UseCorrectCasing.cs
NamingUtils.cs
Test/UseCorrectCasing.cs
-- now handles props and special names right (at lease I hope so), though
with some workarounds (need help here) + new test cases
[*] UseCorrectSuffixRule.cs
-- now it doesn't show error messages like 'type name should end with one of
following suffixes: Collection, Collection, Collection' - not being really an
error,
that looked bad.
Original comment by dan.abra...@gmail.com
on 24 Dec 2007 at 4:29
Attachments:
Ok, I cannot merge this again to test (it's too time consuming without patches)
so
I'll only comment on (parts of) this version.
First I think you forgot to look at Mono.Cecil.MethodSemanticsAttributes ;-)
This code snippet...
string name = methodDefinition.Name;
if (name.StartsWith ("get_") || name.StartsWith ("set_")) {
name = name.Substring (4);
// we NEED to check properties though they're generated by compiler
} else if (NamingUtils.IsNameGeneratedByCompiler (methodDefinition)) {
return runner.RuleSuccess;
}
will fail if I
* define methods that are named get_ or set_ (with or without a suffix);
* the getter/setter was generated by the compiler (I don't know any C# case but
it's
a possibility, e.g. another compiler), i.e. the check should be before
it also doesn't handle the case for add_ and remove_ (MethodSemanticsAttributes
again) which aren't related to operator overloading (good test case btw).
[As a "rule" always check for rule applicability before starting the rule
logic. This
helps to keep the rules readable (and this will become mandatory in the next
version
of Gendarme)]
Also
* can you tell me why you added RemoveLeadingUnderlines ? the current code, if
needed, will potentially (and needlessly) allocate a lot of strings.
* what are the false positives for which you commented code inside NamingUtils ?
* there's still code duplication inside NamingUtils to check for attributes.
Original comment by sebastie...@gmail.com
on 24 Dec 2007 at 5:23
> First I think you forgot to look at Mono.Cecil.MethodSemanticsAttributes ;-)
OK.
> can you tell me why you added RemoveLeadingUnderlines ? the current code, if
needed, will potentially (and needlessly) allocate a lot of strings.
Because otherwise I get error messages like "replace '_blabla' identifier with
'_blabla'" but not the intended "replace '_blabla' identifier with 'Blabla'".
> what are the false positives for which you commented code inside NamingUtils ?
If you build with that code, unit tests will fail for CorrectCasing (good
pascal-cased class) and something else (don't remember what exactly).
> there's still code duplication inside NamingUtils to check for attributes.
OK.
Original comment by dan.abra...@gmail.com
on 24 Dec 2007 at 6:36
Perhaps, I should better use StringBuilder instead (speaking of underlines in
NamingUtils).
Original comment by dan.abra...@gmail.com
on 24 Dec 2007 at 6:38
If the check is really needed(*) then it would be better to loop to find the
first
non _ char then do a single String.Substring call
(*) there's already a rule to report _ in names
p.s. I'll try your unit tests with your newer code too see the CorrectCasing
failure
Original comment by sebastie...@gmail.com
on 24 Dec 2007 at 6:46
I get 66 (all) success using
// triggers false positives -- why?
if (type.IsNested)
return IsGeneratedByCompiler (type) || IsGeneratedByCompiler (type.DeclaringType);
// so as of now we use this ugly replacement
// if (type.Name.Contains ("$") || type.Name.Contains ("<") ||
type.Name.Contains
(">"))
// return true;
This site is an horrible chat room. Is it possible for you to get into IRC ? If
so
can you join #gendarme on GIMPnet, my nick is spouliot. Thanks
Original comment by sebastie...@gmail.com
on 24 Dec 2007 at 6:55
Hope it's last version :-)
It's now like SVN + patch in #16 + copy this, if I am right.
Original comment by dan.abra...@gmail.com
on 24 Dec 2007 at 10:04
Attachments:
Rules and test have been committed to SVN (r91901/2).
You may want to take a look to be sure I didn't merge incorrectly.
Great job! Thanks and enjoy your vacations ;-)
note: switching owner and closing task.
Original comment by sebastie...@gmail.com
on 26 Dec 2007 at 6:48
Original issue reported on code.google.com by
jbev...@gmail.com
on 21 Nov 2007 at 10:09