Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code Style: Initialize Objects with Properties #143

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

meisenzahl
Copy link

Closes #142

@meisenzahl meisenzahl marked this pull request as ready for review November 12, 2020 19:44
@meisenzahl
Copy link
Author

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 🤓

@kgrubb kgrubb requested a review from pantor November 19, 2020 20:01
@meisenzahl
Copy link
Author

I would be really happy about a review 😃️

@jeremypw
Copy link
Collaborator

jeremypw commented May 28, 2021

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.

@kgrubb
Copy link
Collaborator

kgrubb commented Apr 1, 2022

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?

@avojak
Copy link
Contributor

avojak commented Jun 24, 2022

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.

@jeremypw
Copy link
Collaborator

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.

@pantor pantor removed their request for review June 21, 2024 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code Style: Initialize Objects with Properties
4 participants