ConsiderReplacingPlatformSpecificPathsRule
Exception
I've made few changes in code and haven't yet updated one or two
comments.
Sorry for such a _short_ message, I really have to go right now :-)
See you later!
http://gendarme.googlegroups.com/web/consider_replacing_paths.patch
Cedric Vivier
Nice rule!
Shouldn't the rule name ends with singular 'Path' instead?
Anyways it stresses again (as discussed many times I guess) how (enforced) rule naming guidelines are needed :-)
[gendarme rules sometimes uses plural sometime singular, sometime 'DoNot', sometime 'Dont', etc...]
+ OpenFile (42, "/home/ex/.aMule/Incoming/Blues_Brothers.avi", "yarr!");Fe4r th3 MPAA!
+ [Solution ("Use System.IO.Path methods and properties to work with paths instead of hardcoding them.")]Maybe "Use System.IO.Path and System.Environment types to generate paths instead of hardcoding them." would be a better message ?Regards,
Sebastien Pouliot
I think this "old" comment still applies ;-)
<quote from="an email about the previous rule">
About the tests (I'm not sure if we discussed it before or if I just
took some notes about it) would it be possible not to add overloads
for Confidence(*) but to have an Assert"Something" that can give us
access to the runner (and it's defects). E.g.
AssertDefect (int n, Confidence?, Severity?, string?
containsDetails)
(*) we'll eventually need the same for Severity and, for both cases,
we need to support different values for each defects. We also need the
same support for Type and Assembly... so it looks like a lot of
(future) duplicated code.
</quote>
Except that this time I'm sure we talked about it ;-)
I'll look (and try) the rule later tonight or, worse case, this weekend.
Thanks!
Sebastien
Exception
On 30 май, 02:08, "Cedric Vivier" <cedr...@neonux.com> wrote:
> Hi!
>
> Nice rule!
Thanks!
> Shouldn't the rule name ends with singular 'Path' instead?
> instead of hardcoding them." would be a better message ?
messages :-) . Thanks for your suggestions!
> Anyways it stresses again (as discussed many times I guess) how (enforced)
> rule naming guidelines are needed :-)
convention.
> I think this "old" comment still applies ;-)
Thank you all for the comments :) !
Exception
I've done some testing and fixed lots of false positives. Also as of
now the rule can check field and property assignments.
There is a bug (look for FIXME:) that does not allow me to know
parameter names in case the method is located in a different assembly
(I have no idea how to cope this). Also I'm afraid the rule is slow so
we'll either need to optimize it (I did my best to make it work faster
but I've no experience in optimization so probably Seb's or someone
else's help is the best option available).
P.S. I haven't yet updated the Test.Rules stuff (and I will, don't
worry Seb :D), that will come later
P.P.S. I'm going outta the city before the 3rd of June (it's Bob
Dylan's concert!) so you won't be able to contact me before that
time :-) . Feel free to post your suggestions here, as I'm going to
return only for 3.06 and then go back to the country for ~ half a week
(and I hope I'll find some time there to write the rule(s)).
http://groups.google.com/group/gendarme/web/consider_replacing_path_r2.patch
Sebastien Pouliot
On Sat, 2008-05-31 at 03:30 -0700, Exception wrote:
> Hello again :-)
> I've done some testing and fixed lots of false positives. Also as of
> now the rule can check field and property assignments.
> There is a bug (look for FIXME:) that does not allow me to know
> parameter names in case the method is located in a different assembly
> (I have no idea how to cope this). Also I'm afraid the rule is slow so
> we'll either need to optimize it (I did my best to make it work faster
> but I've no experience in optimization so probably Seb's or someone
> else's help is the best option available).
Don't worry about it. First we make it right, then we make it fast.
I had a look at the previous source and there seems to be a lot of
potential for nice rocks (wrt to existing rules). Not that it will make
it faster, but maybe smaller ;-).
> P.S. I haven't yet updated the Test.Rules stuff (and I will, don't
> worry Seb :D), that will come later
Cool :)
> P.P.S. I'm going outta the city before the 3rd of June (it's Bob
> Dylan's concert!) so you won't be able to contact me before that
> time :-) . Feel free to post your suggestions here, as I'm going to
> return only for 3.06 and then go back to the country for ~ half a week
Have fun, both times!
> (and I hope I'll find some time there to write the rule(s)).
>
> http://groups.google.com/group/gendarme/web/consider_replacing_path_r2.patch
I'll be checking this very soon :)
Thanks
Sebastien
Sebastien Pouliot
So how was the show ? :)
Some comments inline, others in the attached code...
On Sat, 2008-05-31 at 03:30 -0700, Exception wrote:
> Hello again :-)
> I've done some testing and fixed lots of false positives.
I executed your rule against mono 2.0 assemblies and the results are
good. There are false positives but they, mostly, fit the "filename"
description pretty well.
The most common of the false positives were from MIME type definitions -
so we might need to check if we can reduce them (in numbers or in
confidence).
> Also as of
> now the rule can check field and property assignments.
> There is a bug (look for FIXME:) that does not allow me to know
> parameter names in case the method is located in a different assembly
> (I have no idea how to cope this).
My initial guess was (and still is*) that you were missing a Resolve
call.
* This is the part I have not looked yet ;-)
> Also I'm afraid the rule is slow so
> we'll either need to optimize it (I did my best to make it work faster
> but I've no experience in optimization so probably Seb's or someone
> else's help is the best option available).
It's not that bad - but could be better.
First the Regex is probably the main performance offender. It needs
timing (like any optimization) but it's the first thing to jump to mind.
A second one is based on the fact that all the strings loaded by LDSTR
are located inside a table. Accessing it is probably a bit too complex
(and subject to change API wise). However I think it's very likely that
many strings are loaded more than once, meaning that you're processing
them more often than required.
Caching the result, e.g. Dictionary<string,Confidence?>, seems like a
potential gain (needs measurements too) and, as a bonus, does not
require much change in your rule logic :)
> P.S. I haven't yet updated the Test.Rules stuff (and I will, don't
> worry Seb :D), that will come later
It makes it a bit harder to test, and harder still to commit ;-)
> P.P.S. I'm going outta the city before the 3rd of June (it's Bob
> Dylan's concert!) so you won't be able to contact me before that
> time :-) . Feel free to post your suggestions here, as I'm going to
> return only for 3.06 and then go back to the country for ~ half a week
> (and I hope I'll find some time there to write the rule(s)).
Sorry for the delay. I was hoping to catch you online :)
> http://groups.google.com/group/gendarme/web/consider_replacing_path_r2.patch
Try to attach patches to the email when possible, it's easier to review
since I can edit it directly.
Thanks!
Sebastien
Exception
On 7 июн, 04:02, Sebastien Pouliot <sebastien.poul...@gmail.com>
wrote:
>
> So how was the show ? :)
anyways, there was a lack of energy, as if he was playing because he
was told to, not because he wanted to. But, still it was very cool to
see him :-) !
>
> Some comments inline, others in the attached code...
>
> On Sat, 2008-05-31 at 03:30 -0700, Exception wrote:
> > Hello again :-)
> > I've done some testing and fixed lots of false positives.
>
> I executed your rule against mono 2.0 assemblies and the results are
> good. There are false positives but they, mostly, fit the "filename"
> description pretty well.
>
> The most common of the false positives were from MIME type definitions -
> so we might need to check if we can reduce them (in numbers or in
> confidence).
>
> > Also as of
> > now the rule can check field and property assignments.
> > There is a bug (look for FIXME:) that does not allow me to know
> > parameter names in case the method is located in a different assembly
> > (I have no idea how to cope this).
>
> My initial guess was (and still is*) that you were missing a Resolve
> call.
>
> * This is the part I have not looked yet ;-)
http://www.seeklyrics.com/lyrics/Foo-Fighters/Resolve.html
>
> > Also I'm afraid the rule is slow so
> > we'll either need to optimize it (I did my best to make it work faster
> > but I've no experience in optimization so probably Seb's or someone
> > else's help is the best option available).
>
> It's not that bad - but could be better.
>
> First the Regex is probably the main performance offender. It needs
> timing (like any optimization) but it's the first thing to jump to mind.
> A second one is based on the fact that all the strings loaded by LDSTR
> are located inside a table. Accessing it is probably a bit too complex
> (and subject to change API wise). However I think it's very likely that
> many strings are loaded more than once, meaning that you're processing
> them more often than required.
>
> Caching the result, e.g. Dictionary<string,Confidence?>, seems like a
> potential gain (needs measurements too) and, as a bonus, does not
> require much change in your rule logic :)
> > P.S. I haven't yet updated the Test.Rules stuff (and I will, don't
> > worry Seb :D), that will come later
>
> It makes it a bit harder to test, and harder still to commit ;-)
second.
> > P.P.S. I'm going outta the city before the 3rd of June (it's Bob
> > Dylan's concert!) so you won't be able to contact me before that
> > time :-) . Feel free to post your suggestions here, as I'm going to
> > return only for 3.06 and then go back to the country for ~ half a week
> > (and I hope I'll find some time there to write the rule(s)).
>
> Sorry for the delay. I was hoping to catch you online :)
>
> Try to attach patches to the email when possible, it's easier to review
> since I can edit it directly.
message, like you did ConsiderReplacingPlatformSpecificPathRule.cs in
the previous post? NP :)
Thanks!
Dan
>
> Thanks!
> Sebastien
>
> > On 30 май, 18:16, Exception <dan.abra...@gmail.com> wrote:
> > > Hello!
>
> > > On 30 май, 02:08, "Cedric Vivier" <cedr...@neonux.com> wrote:
>
> > > > Hi!
>
> > > > Nice rule!
>
> > > Thanks!
>
> > > > Shouldn't the rule name ends with singular 'Path' instead?
> > > > Maybe "Use System.IO.Path and System.Environment types to generate paths
> > > > instead of hardcoding them." would be a better message ?
>
> > > Yeah, naturally I was sure we'd have to change the rule name and
> > > messages :-) . Thanks for your suggestions!
>
> > > > Anyways it stresses again (as discussed many times I guess) how (enforced)
> > > > rule naming guidelines are needed :-)
>
> > > We could take fashion from FxCop rule names or invent our own naming
> > > convention.
>
> > > > I think this "old" comment still applies ;-)
>
> > > Ah, that was one of those lost changes. I'll fix it tonight.
>
> > > Thank you all for the comments :) !
>
>
>
> 12KЗагрузить
Sebastien Pouliot
On Sat, 2008-06-07 at 02:24 -0700, Exception wrote:
> Hi Seb!
>
> On 7 июн, 04:02, Sebastien Pouliot <sebastien.poul...@gmail.com>
> wrote:
> > Hello Dan,
> >
> > So how was the show ? :)
>
> Well, he's actually getting old. Though it was great to see him live
> anyways, there was a lack of energy, as if he was playing because he
> was told to, not because he wanted to. But, still it was very cool to
> see him :-) !
:)
> >
> > Some comments inline, others in the attached code...
> >
> > On Sat, 2008-05-31 at 03:30 -0700, Exception wrote:
> > > Hello again :-)
> > > I've done some testing and fixed lots of false positives.
> >
> > I executed your rule against mono 2.0 assemblies and the results are
> > good. There are false positives but they, mostly, fit the "filename"
> > description pretty well.
> >
> > The most common of the false positives were from MIME type definitions -
> > so we might need to check if we can reduce them (in numbers or in
> > confidence).
>
> Yeah, I've noticed it. I'll think how to cope this.
I did not look if the variable name gives a hint, or not. Anyway MIME
types would look like relative path with a single '/'. There might be a
few other things to check from the MIME RFC.
> > > Also as of
> > > now the rule can check field and property assignments.
> > > There is a bug (look for FIXME:) that does not allow me to know
> > > parameter names in case the method is located in a different assembly
> > > (I have no idea how to cope this).
> >
> > My initial guess was (and still is*) that you were missing a Resolve
> > call.
> >
> > * This is the part I have not looked yet ;-)
>
> 'A little bit of Resolve () is what I need now...'
> http://www.seeklyrics.com/lyrics/Foo-Fighters/Resolve.html
:)
> > > Also I'm afraid the rule is slow so
> > > we'll either need to optimize it (I did my best to make it work faster
> > > but I've no experience in optimization so probably Seb's or someone
> > > else's help is the best option available).
> >
> > It's not that bad - but could be better.
> >
> > First the Regex is probably the main performance offender. It needs
> > timing (like any optimization) but it's the first thing to jump to mind.
>
> I'll replace it with multiple if's.
>
> > A second one is based on the fact that all the strings loaded by LDSTR
> > are located inside a table. Accessing it is probably a bit too complex
> > (and subject to change API wise). However I think it's very likely that
> > many strings are loaded more than once, meaning that you're processing
> > them more often than required.
> >
> > Caching the result, e.g. Dictionary<string,Confidence?>, seems like a
> > potential gain (needs measurements too) and, as a bonus, does not
> > require much change in your rule logic :)
>
> OK.
>
> > > P.S. I haven't yet updated the Test.Rules stuff (and I will, don't
> > > worry Seb :D), that will come later
> >
> > It makes it a bit harder to test, and harder still to commit ;-)
>
> It's already done on my laptop ;) . I remember: dull first, cool
> second.
;-)
> > > P.P.S. I'm going outta the city before the 3rd of June (it's Bob
> > > Dylan's concert!) so you won't be able to contact me before that
> > > time :-) . Feel free to post your suggestions here, as I'm going to
> > > return only for 3.06 and then go back to the country for ~ half a week
> > > (and I hope I'll find some time there to write the rule(s)).
> >
> > Sorry for the delay. I was hoping to catch you online :)
> >
> > >http://groups.google.com/group/gendarme/web/consider_replacing_path_r...
> >
> > Try to attach patches to the email when possible, it's easier to review
> > since I can edit it directly.
>
> I don't quite get the difference ;-) . Should I attach them to the
> message, like you did ConsiderReplacingPlatformSpecificPathRule.cs in
> the previous post? NP :)
Yes, it's easier to review this way: I just need to reply over the whole
body of the email. It also works without an internet connection (but
that's something less and less problematic these days).
>From an history point of view it will always be in the email, even years
from now. OTOH we're likely to clean up the file section from time to
time.
Daniel Abramov
P.S. I couldn't send the message because I was sending it using e...@vingrad.ru, not dan.a...@gmail.com :)
Best,
Dan