Page MenuHomePhabricator

merge: respect ui.relative-paths for some warning messages
AbandonedPublic

Authored by martinvonz on Thu, Mar 5, 2:58 AM.

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers
Summary

I didn't bother adding tests. It doesn't seem worth having tests for
every place we use getuipathfn(). I've tested it manually.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

martinvonz created this revision.Thu, Mar 5, 2:58 AM
marmoute requested changes to this revision.Thu, Mar 5, 3:32 AM
marmoute added a subscriber: marmoute.

Adding test for this would be better. It clarify the intend and prevent regression.

This revision now requires changes to proceed.Thu, Mar 5, 3:32 AM

Adding test for this would be better. It clarify the intend and prevent regression.

Tests for each of the strings?

I think we are talking about a handful of different string. Are we not?

I think we are talking about a handful of different string. Are we not?

Yes, but I trust the uipathfn to work as it does elsewhere. I'll let reviewers decide to take this as is or not. Otherwise I'll just abandon it.

I am confused. Why can't we get a handful of case added to the test ? I assume these message are alredy tested and would be easy to retest.

Chatted with Raphaël to try to understand where a misunderstanding could lie here.

So to clarify this code path is never tested for in the relative case, my issue is not with this change, but with the lack of testing of these relative path altogether. Without testing of the relative case, any future code update discarding the uipathfn call would silently regress your fix.

Chatted with Raphaël to try to understand where a misunderstanding could lie here.
So to clarify this code path is never tested for in the relative case, my issue is not with this change, but with the lack of testing of these relative path altogether. Without testing of the relative case, any future code update discarding the uipathfn call would silently regress your fix.

Yes, that's correct.

Okay, then we should add tests for the relative case to make sure we do not regress these in the future. Can you do this ?

Okay, then we should add tests for the relative case to make sure we do not regress these in the future. Can you do this ?

Ah, I think I now understand where the confusion comes from. I initially said "I trust the uipathfn to work as it does elsewhere.", but that was only half the truth -- the other half is that I spotted this little bug and decided to fix it, but I don't really care much about it. Sorry I wasn't clear about that part. So I don't care enough to write tests with relative paths for theses cases. (I also don't care enough to add retroactive tests for D5916, D5936, D5937, D5939, D6264, and probably many more. And I definitely care enough to rewrite all our tests to be have more directories and for commands to be run in subdirectories, even though that would have been nice to have.) I'm happy to add tests if someone else writes them, though.

And I definitely care enough to rewrite all our tests to be have more directories and for commands to be run in subdirectories, even though that would have been nice to have.) I'm happy to add tests if someone else writes them, though.

I don't understand that part, is there some negation missing?

And I definitely care enough to rewrite all our tests to be have more directories and for commands to be run in subdirectories, even though that would have been nice to have.) I'm happy to add tests if someone else writes them, though.

I don't understand that part, is there some negation missing?

Oops, should have been "And I *don't* definitely care enough" :P

Since forever, the option relative path option never worked with this output. So, no user should be "surprised" if it don't. However if you fix it people may start to rely on it. Without tests it can regress, breaking user expectation.

I understant this is a side-quest cleanup for you, but I woudl rather see this slow cleanup advance twice as slow with test that at the current pace without tests.

Since forever, the option relative path option never worked with this output. So, no user should be "surprised" if it don't. However if you fix it people may start to rely on it. Without tests it can regress, breaking user expectation.
I understant this is a side-quest cleanup for you, but I woudl rather see this slow cleanup advance twice as slow with test that at the current pace without tests.

nit: s/twice/10 times/

Fine, I am sure you will speed up with time. (I assume all these message are already tested, changing the pwd from where the command run should not be insane)

Fine, I am sure you will speed up with time. (I assume all these message are already tested, changing the pwd from where the command run should not be insane)

I've said before that I'll let other reviewers decide if they want the patch or if I should abandon it. Talking about it more just makes me tired.

martinvonz abandoned this revision.Fri, Mar 13, 1:17 PM