Fix build in place-with-space #145
Conversation
mohawk2
force-pushed
the
fix-in-space
branch
3 times, most recently
from
576d035 to
7e9f420
Compare
|
Text::ParseWords::shellwords is a great idea |
|
@dk Thanks. Please could you merge and release this? |
|
I'd rather wait for you to answer my questions in the review first.. |
The "review" UI is pretty clear about reviews being pending. This means that, as here, you haven't actually sent it yet. Until you do, it will be very significantly difficult for me to answer any questions you may have. |
dk
left a comment
dk
left a comment
There was a problem hiding this comment.
Please see the comments under individual commits
| OBJECT => "@o_files", | ||
| INC => | ||
| join(' ', map { "-I$_" } @INCPATH ). | ||
| join(' ', map qq{"-I$_"}, @INCPATH ). |
There was a problem hiding this comment.
I get the idea, but why not maybe_quote?
There was a problem hiding this comment.
Because that is how EUMM does it (unconditionally), which works great in all environments.
There was a problem hiding this comment.
Fair enough, but I think that conditionally quoted lines look better than unconditionally quoted. I rarely copypaste the individual compilation or linking lines and run them as standalone, and they are mostly ugly enough as they are, without quotes. The idea is good but let's use maybe_quote still, at least it is systematically used throughout makefile.pl
There was a problem hiding this comment.
Dmitry's perspective makes sense to me. I like that in Perl I can say $hash{key} and am not required to say $hash{"key"}. Improving the legibility of auto-generated code is a good thing. Unlike the makefile's $(lib), these are Perl values that can be accurately examined.
|
I don't understand why do you want a formal review instead of just reading comments under the proposed commits, but if it helps, well... here it goes |
Until today the only comment visible on here was "Text::ParseWords::shellwords is a great idea". You said above you'd "rather wait for [me] to answer my questions in the review first" about the changes I made here. No such questions were visible to me or anyone else, at all. Either the "questions" were a GitHub review you'd written but not published (my assumption), or one of us doesn't know how to ask questions. |
|
That's not true that the only comment was visible is the one above; my comments on the commits were visible for 23 days. I also get mails for your answers under them today, so it looks like these were published alright. Now, I added some text under both, hopefully you can see them. In short though, the first commit with 0,1 I don't find necessary, while the 2nd with the qq is okay but I would prefer maybe_quote. If you would address these I shall merge the pr |
Are you saying that I am wrong about what I could see in my view of this PR? You getting emails about my answers is irrelevant to what I was able to see. I can now see you wrote some messages that are dated 29 March, but they only became visible to me in late April.
There are no guarantees you won't change |
|
Hello everybody,
I have been following this thread and thought I might provide a bit of
extra context. Regarding Dmitry's comments on the pull request, I remember
trying to go look at Dmitry's comments back on April 11, because I was
curious. However, I was unable to find them. I still do not know where
those comments live, but for some reason their presence was never reflected
in this issue thread, or even accessible when I tried to do a little bit of
digging.
All this to say, Dmitry, I think the way that you commented on the commits
was somehow obscured. I am pretty sure you've commented on my commits in
previous work and I was notified of those comments. That's a good practice
in general as it provides highly contextualized feedback. I suspect the
problem is that there are multiple ways that one can think they are
commenting on a commit and some of them will be visible while others won't.
For example, if you explored some of Ed's branches in his forked repo and
managed to comment on a branch that wasn't the one he was using for the
pull request? At any rate, I never had visibility into your comments, and
apparently neither did Ed until one of your follow-ups.
David
…On Mon, Apr 28, 2025 at 8:30 AM mohawk2 ***@***.***> wrote:
*mohawk2* left a comment (dk/Prima#145)
<#145 (comment)>
That's not true that the only comment was visible is the one above; my
comments on the commits were visible for 23 days. I also get mails for your
answers under them today, so it looks like these were published alright.
Are you saying that I am wrong about what I could see in my view of this
PR? You getting emails about my answers is irrelevant to what I was able to
see. I can *now* see you wrote some messages that are dated 29 March, but
they only became visible to me in late April.
Now, I added some text under both, hopefully you can see them. In short
though, the first commit with 0,1 I don't find necessary, while the 2nd
with the qq is okay but I would prefer maybe_quote. If you would address
these I shall merge the pr
There are no guarantees you won't change @incpath to do hardcode values
like $(lib) that will be later expanded to have spaces, but which would
give a false negative in maybe_quote. I am therefore leaving the quotes
as unconditional.
—
Reply to this email directly, view it on GitHub
<#145 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB7Q5YJFDVZCOJBYVYPWRT23YNNBAVCNFSM6AAAAAB2AZNMRCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMZVGA4DKOJXGY>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
--
"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." -- Brian Kernighan
|
|
If that's okay, i can also cancel this pr and rework the patch. |
Yes I believe you are wrong in exactly about what you could see after 29/03. I did not do any special privacy editing or similar changes in these comments, and I doubt that it was a github UI bug that hid these from you and only magically made them visible in the later april. I believe these comments were seen immediately, but either the notification didn't reach you properly, or you did look in the wrong place. Anyways, I'm glad that they are visible to you, and indeed as David pointed, they provide much better contextualization for the discussion. I agreed with the arguments about makefile expansion, so this part is okay; but not about the uncoditional quotes, so to expand on my previous short note, if there is any problem with changing the commit, and can also cancel the pr but add the patches manually. |
|
I've incorporated the changes here and added a missed case of shellwords, kindly test |
3 participants
Thanks for the new version! With these changes (plus these fixes for PkgConfig: PerlPkgConfig/perl-PkgConfig#61), Prima now builds cleanly on my "place-with-space" Windows setup.