nano-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Nano-devel] Patch for bug #44950


From: Rishabh Dave
Subject: Re: [Nano-devel] Patch for bug #44950
Date: Wed, 6 Jan 2016 01:07:48 +0530

I thought if I am solving bug for nano, that could only be done using some other text editor! :P



But... determine() returns one of two values, either -1 or 0
-- why not make it a bool?  Also, you assign the result of
determine() to m, but never use this variable again, so...
better get rid of it.


Both, done. However, there is more on determine()'s return type, below.



More importantly, your patch isn't a unified diff -- it is hard
to read and impossible to apply.  Please use 'diff -ur'.


Did 'diff -ur' this time.



And finally, when running './nano foo/bar/baz', with your patch
nano would say that dir 'foo/bar' does not exist, whereas I would
expect it to say that dir 'foo' does not exist.  I admit, it is
harder to make, but the message would be nicer, more natural.

I achieved it but I think code might be little ugly, so I added comments over every if-statements explaining them quickly. I have following concerns regarding patch, I am putting them as follows -

After changing function's return type to bool and after covering case of locking files, it struck me it will be more appropriate to name it 'fault_in_path()'. It would also make code readable, for example - b = fault_in_path(a/b/c/d). Feeling this was appropriate I changed name of our variable 'mistakes_in_path' to 'faulty_path'. Even that is more readable now, I suppose (e.g. faulty_path = TRUE). Hopefully that holds true for anyone who will read code.

The major change apart from making message specific, was changing return type of fault_in_path() - which was determine() before. Code responsible for file locking already checks if path exists and displays incorrect path ('a/b/c doesn't exist', the way last time I did). I felt it would be more convenient to change that section now but I also felt little awkward towards changing the code there, so there are two patch files attached - basic, doesn't makes message specific for -G option, extended does. However, both patch contains code that changes center section of titlebar to "New Buffer", for -G option too.

Now, these ones are only trivial. It is not possible to detect multiple errors in path, only outermost incorrect directory can be detected. There is no economic way of even guessing multiple incorrect directories, right? Other thing, is it good to re-use variables in same function for same purpose but over different objects? I chose not to for fault_in_path() (len1 could be used for two objects rendering len3 redundant).

And... Am I asking too many questions?

Attachment: find-nonexistent-dir-basic.patch
Description: Text Data

Attachment: find-nonexistent-dir-extended.patch
Description: Text Data


reply via email to

[Prev in Thread] Current Thread [Next in Thread]