You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/2256
-- Commit Summary --
* ASSIGNIF -> assign_cmd
-- File Changes --
M src/build.c (43)
-- Patch Links --
https://github.com/geany/geany/pull/2256.patch https://github.com/geany/geany/pull/2256.diff
LarsGit223 commented on this pull request.
@@ -2291,6 +2291,22 @@ static void build_load_menu_grp(GKeyFile *config, GeanyBuildCommand **dst, gint
}
+/* set GeanyBuildCommand if it doesn't already exist and there is a command */ +static void assign_cmd(GeanyBuildCommand *type, guint id, + const gchar *label, gchar *value) +{ + if (!EMPTY(value) && ! type[GBO_TO_CMD(id)].exists) + { + type[GBO_TO_CMD(id)].exists = TRUE; + SETPTR(type[GBO_TO_CMD(id)].label, g_strdup(label)); + SETPTR(type[GBO_TO_CMD(id)].command, value); + SETPTR(type[GBO_TO_CMD(id)].working_dir, NULL); + type[GBO_TO_CMD(id)].old = TRUE; + } + else + g_free(value);
I know it was not in the original code but I would like brackets around the else branch even if it's just one line.
ntrel commented on this pull request.
@@ -2291,6 +2291,22 @@ static void build_load_menu_grp(GKeyFile *config, GeanyBuildCommand **dst, gint
}
+/* set GeanyBuildCommand if it doesn't already exist and there is a command */ +static void assign_cmd(GeanyBuildCommand *type, guint id, + const gchar *label, gchar *value) +{ + if (!EMPTY(value) && ! type[GBO_TO_CMD(id)].exists) + { + type[GBO_TO_CMD(id)].exists = TRUE; + SETPTR(type[GBO_TO_CMD(id)].label, g_strdup(label)); + SETPTR(type[GBO_TO_CMD(id)].command, value); + SETPTR(type[GBO_TO_CMD(id)].working_dir, NULL); + type[GBO_TO_CMD(id)].old = TRUE; + } + else + g_free(value);
I don't think that helps readability here. If you want to make a pull against my branch I'll merge it though.
LarsGit223 commented on this pull request.
@@ -2291,6 +2291,22 @@ static void build_load_menu_grp(GKeyFile *config, GeanyBuildCommand **dst, gint
}
+/* set GeanyBuildCommand if it doesn't already exist and there is a command */ +static void assign_cmd(GeanyBuildCommand *type, guint id, + const gchar *label, gchar *value) +{ + if (!EMPTY(value) && ! type[GBO_TO_CMD(id)].exists) + { + type[GBO_TO_CMD(id)].exists = TRUE; + SETPTR(type[GBO_TO_CMD(id)].label, g_strdup(label)); + SETPTR(type[GBO_TO_CMD(id)].command, value); + SETPTR(type[GBO_TO_CMD(id)].working_dir, NULL); + type[GBO_TO_CMD(id)].old = TRUE; + } + else + g_free(value);
IMHO it would but I leave the decision up to you. Fine for me if you prefer it that way.
Looks ok by quick inspection, havn't tested.
kugel- requested changes on this pull request.
I think the commit message lacks a bit of story. Yes, the change fixes a TODO in the code but the message could mention just that.
@@ -2291,6 +2291,22 @@ static void build_load_menu_grp(GKeyFile *config, GeanyBuildCommand **dst, gint
}
+/* set GeanyBuildCommand if it doesn't already exist and there is a command */ +static void assign_cmd(GeanyBuildCommand *type, guint id, + const gchar *label, gchar *value) +{ + if (!EMPTY(value) && ! type[GBO_TO_CMD(id)].exists) + { + type[GBO_TO_CMD(id)].exists = TRUE; + SETPTR(type[GBO_TO_CMD(id)].label, g_strdup(label)); + SETPTR(type[GBO_TO_CMD(id)].command, value); + SETPTR(type[GBO_TO_CMD(id)].working_dir, NULL); + type[GBO_TO_CMD(id)].old = TRUE;
The readablity could be improved by using a temporary variable for `GBO_TO_CMD(id)` or even the whole `type[GBO_TO_CMD(id)`
ntrel commented on this pull request.
@@ -2291,6 +2291,22 @@ static void build_load_menu_grp(GKeyFile *config, GeanyBuildCommand **dst, gint
}
+/* set GeanyBuildCommand if it doesn't already exist and there is a command */ +static void assign_cmd(GeanyBuildCommand *type, guint id, + const gchar *label, gchar *value) +{ + if (!EMPTY(value) && ! type[GBO_TO_CMD(id)].exists) + { + type[GBO_TO_CMD(id)].exists = TRUE; + SETPTR(type[GBO_TO_CMD(id)].label, g_strdup(label)); + SETPTR(type[GBO_TO_CMD(id)].command, value); + SETPTR(type[GBO_TO_CMD(id)].working_dir, NULL); + type[GBO_TO_CMD(id)].old = TRUE;
I was trying not to make several changes at once, to avoid bugs.
There are other refactorings for build.c of higher priority IMO than adding braces or refactoring expressions in assign_cmd (which is short). I support using an intermediate variable for common expressions, but this should be done for bigger functions in build.c first.
Why even post a PR if you're reluctant to even minor suggestions? I guess responding in a comment here takes about the same time as following the suggestion (that you generally support). So if this is your reaction to review comments please just push directly, this has the same result and saves us time (I just wasted my time by reviewing this and writing this comment.), and have the thing reverted if anyone bothers enough (I doubt).
I'm trying to be not rude (really!) but this is my personal impression of this PR: "Hey, I prepared this change with a poor title and no description and code changes that leave room for improvements. But better not request changes because it's wasting my time and there is higher priority stuff elsewhere".
I support using an intermediate variable for common expressions, but this should be done for bigger functions in build.c first.
This doesn't make a lot of sense to me. You edited this code and it could be improved while you're at it. Why avoid improving this code just because there are other, bigger functions that can also be improved?
And why are you even doing this change at all if there are other refactoring of higher priority?
I'll edit the commit message before merging.
Why even post a PR
So reviewers can check my changes don't introduce bugs.
if you're reluctant to even minor suggestions?
I learnt that refactoring should focus on doing one thing well. Making multiple changes at once increases the risk of bugs, and makes it harder to review.
If I had written this code, then these review suggestions are fine and I would make the changes (I already thought of the intermediate variable and decided not to do it as explained above).
why are you even doing this change at all if there are other refactoring of higher priority?
I felt like removing this macro. Removing this macro is more important than refactoring assign_cmd.
Also, I don't always have access to my development machine ATM, so this is a factor for me.
Sorry you feel you wasted your time. When reviewing code though, it's good to bear in mind the difference between new code and refactoring. Also suggesting small changes is actually adding work for a volunteer, consider whether the changes are good enough to accept without the suggestions implemented. Working on dlang, I often saw 'I suggest doing X, but it's not a blocker'. This keeps a good balance between submitter and reviewer. Let's be pragmatic.
@kugel- said:
Why even post a PR if you're reluctant to even minor suggestions?
There is nothing wrong with making suggestions for improvements "while you are there" but it also should be ok for contributors to say "not just now". The Geany crew (me included) have a tendency to make perfection the enemy of improvement.
Of course we should object to bad changes, changes that cause problems or are poorly implemented, even changes that we simply don't like, but that shouldn't stop changes that are reasonable interim steps being made, even if they are not the final state we would prefer.
This change isn't hugely urgent, since the code being changed is working its only a safety net changing a macro to a function, removing the risk of local names from later changes being captured. This would be important if there were regular changes in the code using it, but there have not been many changes there.
But the change does make an improvement, so why not if its an itch @ntrel wants to scratch.
@ntrel said:
So reviewers can check my changes don't introduce bugs.
Yes, please do NOT commit directly, although review and test can be annoying and takes time its important. We don't have a big enough crew to have to do the extra effort to go back and find and fix accidentally introduced bugs.
And we all know somebody else testing _always_ tries something we didn't think of :grin:
Alright, code change asside (which seems technically correct), I still object to the lack of description and poor title. You can edit it in the Github Webinterface before merging.
codebrainz commented on this pull request.
@@ -2291,6 +2291,22 @@ static void build_load_menu_grp(GKeyFile *config, GeanyBuildCommand **dst, gint
}
+/* set GeanyBuildCommand if it doesn't already exist and there is a command */ +static void assign_cmd(GeanyBuildCommand *type, guint id, + const gchar *label, gchar *value) +{ + if (!EMPTY(value) && ! type[GBO_TO_CMD(id)].exists) + { + type[GBO_TO_CMD(id)].exists = TRUE; + SETPTR(type[GBO_TO_CMD(id)].label, g_strdup(label)); + SETPTR(type[GBO_TO_CMD(id)].command, value); + SETPTR(type[GBO_TO_CMD(id)].working_dir, NULL); + type[GBO_TO_CMD(id)].old = TRUE; + } + else + g_free(value);
IMO, the braceless dangling elses hurt readability and are [a trap for future people editing the code](https://dwheeler.com/essays/apple-goto-fail.html), but it seems perfectly reasonably to not change this one, since it's existing code and IIRC there are many of them elsewhere in the code, so it would probably make more sense to get them all at once in a separate PR that can get bikeshedded over.
Other than the cryptically terse commit message, this looks OK to me.
b4n approved this pull request.
LGTM.
I basically agree with most of what was said, both that this could be improved, and that it's still a worthy change as it's a step forward.
Merged #2256 into master.
github-comments@lists.geany.org