vala-lint icon indicating copy to clipboard operation
vala-lint copied to clipboard

Code Style: Initialize Objects with Properties

Open meisenzahl opened this issue 4 years ago • 6 comments

Closes https://github.com/vala-lang/vala-lint/issues/142

meisenzahl avatar Nov 12 '20 11:11 meisenzahl

It works, but I would really like to get feedback on the implementation of check.

I am merging the single texts from parse_result because I check the whole text with a Regex. There must be a better solution 🤓

meisenzahl avatar Nov 12 '20 19:11 meisenzahl

I would be really happy about a review 😃️

meisenzahl avatar May 27 '21 10:05 meisenzahl

As I understand it, this looks for a pair of lines in the form

<variable> = new <Object> (<parameters>);
<variable>.<property> = <value>;

If so, you could examine pairs of lines instead of the whole contents using two Regexes. If the first of the pair matches the first Regex then test whether the second line matches the second Regex. It would make the Regexes a bit simpler but whether it is a worthwhile change I am not sure.

Neither method will pick up cases where the form <variable>.<property_setter> (<value>); has been used or a property has been set later that could have been set on construction of course.

jeremypw avatar May 28 '21 12:05 jeremypw

Unfortunately, I'm not sure who may be a good person to review this change, and I don't have the knowledge here to provide any good feedback. @danrabbit, do you have any advice on who may be a good person to review this pr?

kgrubb avatar Apr 01 '22 22:04 kgrubb

Did a quick test of this against some old code of mine, and while it picked up a lot, it misses cases where the instantiation doesn't occur in the same line as the declaration. For example, the following is not picked up:

public class MyTest {
    private Gtk.Button button;

    construct {
        button = new Gtk.Button.with_label ("Push me");
        button.valign = Gtk.Align.CENTER;
    }
}

I wonder if the init group in the regex is really necessary?

Another valid case to consider would be the use of the prefix this.

avojak avatar Jun 24 '22 03:06 avojak

I guess more tests need to be added to cover the missed cases mentioned above and more work put into the code to fail these cases.

jeremypw avatar Jun 24 '22 08:06 jeremypw