Merged #2256 into master.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/2256#event-2563899746
Other than the cryptically terse commit message, this looks OK to me.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/2256#issuecomment-521314912
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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on
@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
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
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
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.
--
You are receiving
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,
+
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
}
Looks ok by quick inspection, havn't tested.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/2256#issuecomment-520661188
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
11 matches
Mail list logo
