Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix typos in AllLinkDatabase #41

Merged
merged 1 commit into from
Jan 19, 2013
Merged

Conversation

krkeegan
Copy link
Collaborator

ALDB was accidentally typed ADLB in a few places. These are likely causing some unnoticed errors, particularly with sorting.

Again, for some reason, 1222-24 and 1244-45 show as changed. Maybe Marc and I use different line endings??

ALDB was accidently typed ADLB in a few places.  These are likely causing some unnoticed errors, particularly with sorting.
@hollie
Copy link
Owner

hollie commented Jan 18, 2013

Regarding line endings: it might help to look into this info page:
https://help.github.com/articles/dealing-with-line-endings

It is important that the settings are correct depending on the platform you use. Otherwise we indeed get diffs that are marked as different due to non-matching line endings.

@mstovenour
Copy link
Collaborator

I’ve found that single line “diffs” that don’t visually look different are caused by tabs vs spaces. Line endings are ignored by git right now for diffs and most modern editors will not mix line endings in the same file. A line ending problem will show up as a modification of the whole file. Single lines are white space. You can prove it to yourself by running “git diff -b filename” on the file. If it ignores the troublesome line then you have a white space issue. Here is an example where I replaced one of the tabs on a line with 8 spaces:

$ git diff lib/Insteon/AllLinkDatabase.pm

diff --git a/lib/Insteon/AllLinkDatabase.pm b/lib/Insteon/AllLinkDatabase.pm

index 65e9304..1304fd1 100755

--- a/lib/Insteon/AllLinkDatabase.pm

+++ b/lib/Insteon/AllLinkDatabase.pm

@@ -2216,7 +2216,7 @@ sub get_first_empty_address

                            $low_address = $new_address if $new_address < $low_address;

                    }

            }
  •           $first_address = ($low_address > 0) ? sprintf('%04X', $low_address - 8) : 0;
    
  •           $first_address = ($low_address > 0) ? sprintf('%04X', $low_address - 8) : 0;
    
    
    
    }
    

mstovenour@MSTOVENOUR-LTX ~/perl/misterhouse.git

$ git help diff

mstovenour@MSTOVENOUR-LTX ~/perl/misterhouse.git

$ git diff -w lib/Insteon/AllLinkDatabase.pm

mstovenour@MSTOVENOUR-LTX ~/perl/misterhouse.git

We could have a whole thread on how to manage indentation…….. I’ve decided to follow Bruce Winter’s lead and set my editor to use tab characters and visually space tabs as 8 characters. This is not my personal preference but it does indent the code to where I can read it. I use Eclipse with EPIC and it was not easy to coerce the editor to this mode I had to change the Eclipse configuration in multiple places. Also, some editors have an insidious behavior where their “tab key” indention is a tab but the auto indention logic inserts spaces. I found this mixed use of tabs and spaces on the same line in the Insteon modules.

From: Lieven Hollevoet [mailto:[email protected]]
Sent: Friday, January 18, 2013 2:46 AM
To: hollie/misterhouse
Subject: Re: [misterhouse] Fix typos in AllLinkDatabase (#41)

Regarding line endings: it might help to look into this info page:
https://help.github.com/articles/dealing-with-line-endings

It is important that the settings are correct depending on the platform you use. Otherwise we indeed get diffs that are marked as different due to non-matching line endings.


Reply to this email directly or view it on GitHub #41 (comment) .

https://github.com/notifications/beacon/Nvoh7bM6g31WX2NN_lw41X9PrrIaPmy8TsOUeVFGdL_41-nIx2kK-ECUJt3hLzvL.gif

@krkeegan
Copy link
Collaborator Author

So I think we will want to resolve the cause of this, otherwise we may
continue to get spurious "false edits."

Since these particular edits were so small I made them in pico. But I did
not alter the lines in question at all. So whatever is happening
is occurring automatically in pico and is only affecting edits made by
Marc.

I agree that git thinks there is a white space difference, however, I can't
find one.

I pulled both versions and view them in Notepad++ which will optionally
show whitespaces and tabs. I don't see any difference on these lines, both
files use tabs. I am not aware of one, but are there multiple characters
for tabs similar to newlines?

@mstovenour
Copy link
Collaborator

Interesting. I forgot about that Notepad++ feature. There is the “show all characters” mode that will show line endings, tabs, and spaces. Plus, I suppose, any other control characters. I’ve only had one instance where I couldn’t figure out what was different by just cursor’ing around the line. I just used the “diff” feature in Eclipse to copy the line from the original file. If that happens again I’ll use Notepad++ to figure it out. I think that happened before I was able to coerce Eclipse into always using tabs.

If you start looking at line endings you will go mad. They are highly inconsistent in the MH files. I’ve never complained since both windows and linux ports of Perl seem to handle it all in stride. If we “fix” the line endings it will forever look like a giant replacement of many files to the diff tools. It will be difficult to go back and diff old versions of a file to new ones.

Which specific file / commit is a good example of your issue?

From: krkeegan [mailto:[email protected]]
Sent: Friday, January 18, 2013 12:55 PM
To: hollie/misterhouse
Cc: Michael Stovenour
Subject: Re: [misterhouse] Fix typos in AllLinkDatabase (#41)

So I think we will want to resolve the cause of this, otherwise we may
continue to get spurious "false edits."

Since these particular edits were so small I made them in pico. But I did
not alter the lines in question at all. So whatever is happening
is occurring automatically in pico and is only affecting edits made by
Marc.

I agree that git thinks there is a white space difference, however, I can't
find one.

I pulled both versions and view them in Notepad++ which will optionally
show whitespaces and tabs. I don't see any difference on these lines, both
files use tabs. I am not aware of one, but are there multiple characters
for tabs similar to newlines?


Reply to this email directly or view it on GitHub #41 (comment) .

https://github.com/notifications/beacon/J6T91GIPIyhU-8ti4GCGP49g6Yzc-NhLegWj1GR-WW5B_uyeSMdC1Pa7S39S3UlC.gif

@krkeegan
Copy link
Collaborator Author

Here look at this commit:
https://github.com/hollie/misterhouse/pull/41/files

Lines 1222-24 and 1244-45 show as changed. I made copies of thethe raw
AllLinkDatabase.pm before and after the commit and looked at them in
Notepad++. I honestly don't see the difference. I agree that git thinks
it is a whitespace issue, but I can't figure out what whitespace. Using
the compare plugin in Notepad++ only shows changes on 1301.

I may have gone mad already.

On Fri, Jan 18, 2013 at 12:30 PM, Michael Stovenour <
[email protected]> wrote:

Interesting. I forgot about that Notepad++ feature. There is the “show all
characters” mode that will show line endings, tabs, and spaces. Plus, I
suppose, any other control characters. I’ve only had one instance where I
couldn’t figure out what was different by just cursor’ing around the line.
I just used the “diff” feature in Eclipse to copy the line from the
original file. If that happens again I’ll use Notepad++ to figure it out. I
think that happened before I was able to coerce Eclipse into always using
tabs.

If you start looking at line endings you will go mad. They are highly
inconsistent in the MH files. I’ve never complained since both windows and
linux ports of Perl seem to handle it all in stride. If we “fix” the line
endings it will forever look like a giant replacement of many files to the
diff tools. It will be difficult to go back and diff old versions of a file
to new ones.

Which specific file / commit is a good example of your issue?

From: krkeegan [mailto:[email protected]]
Sent: Friday, January 18, 2013 12:55 PM
To: hollie/misterhouse
Cc: Michael Stovenour
Subject: Re: [misterhouse] Fix typos in AllLinkDatabase (#41)

So I think we will want to resolve the cause of this, otherwise we may
continue to get spurious "false edits."

Since these particular edits were so small I made them in pico. But I did
not alter the lines in question at all. So whatever is happening
is occurring automatically in pico and is only affecting edits made by
Marc.

I agree that git thinks there is a white space difference, however, I
can't
find one.

I pulled both versions and view them in Notepad++ which will optionally
show whitespaces and tabs. I don't see any difference on these lines, both
files use tabs. I am not aware of one, but are there multiple characters
for tabs similar to newlines?


Reply to this email directly or view it on GitHub <
https://github.com/hollie/misterhouse/pull/41#issuecomment-12435892> .

<
https://github.com/notifications/beacon/J6T91GIPIyhU-8ti4GCGP49g6Yzc-NhLegWj1GR-WW5B_uyeSMdC1Pa7S39S3UlC.gif>


Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-12439662.

@marcmerlin
Copy link
Collaborator

Ok, there are a few whitespace issues, but honestly this fixes some real bugs, so I'm ok putting it as is. Whitespace can be fixed as followup if it's really important.
Thanks for fixing those typos

marcmerlin added a commit that referenced this pull request Jan 19, 2013
Fix typos in AllLinkDatabase
@marcmerlin marcmerlin merged commit 600d355 into hollie:master Jan 19, 2013
@mstovenour
Copy link
Collaborator

OK. Pico did us a favor here. It corrected those lines which had only LF after deciding that the file was in CR / LF line ending format.

Mixing endings in the same file is a bad thing and pico helped out here. It is one thing to have some files in each format but files where the format is mixed in the same file is a bad thing. Editors have different behaviors for detecting the line ending format for a file. Some look only at the first line, some only at the last line, some will look for “any” line containing CR / LF and assume the whole file is DOS, some the opposite. All the well behaved editors are ok so long as the entire file has the same line endings but start mixing it up in the same file and the experience changes dramatically. At some point someone used an editor on that file that was not well behaved.

As far as I know vim, Notepad++, and Eclipse are all good for keeping the line endings the same. I suppose pico is as well based on this. My only reservation is that pico might be using a simple algorithm here that will blow up on us. If pico detects the file is one format (e.g. because the first line was that format) and then proceeds to rewrite the entire file with that format; we will ultimately have trouble. It should be easy to detect though because the whole file will look changed (minus one or just a few lines).

From: krkeegan [mailto:[email protected]]
Sent: Friday, January 18, 2013 2:45 PM
To: hollie/misterhouse
Cc: Michael Stovenour
Subject: Re: [misterhouse] Fix typos in AllLinkDatabase (#41)

Here look at this commit:
https://github.com/hollie/misterhouse/pull/41/files

Lines 1222-24 and 1244-45 show as changed. I made copies of thethe raw
AllLinkDatabase.pm before and after the commit and looked at them in
Notepad++. I honestly don't see the difference. I agree that git thinks
it is a whitespace issue, but I can't figure out what whitespace. Using
the compare plugin in Notepad++ only shows changes on 1301.

I may have gone mad already.

On Fri, Jan 18, 2013 at 12:30 PM, Michael Stovenour <
[email protected]> wrote:

Interesting. I forgot about that Notepad++ feature. There is the “show all
characters” mode that will show line endings, tabs, and spaces. Plus, I
suppose, any other control characters. I’ve only had one instance where I
couldn’t figure out what was different by just cursor’ing around the line.
I just used the “diff” feature in Eclipse to copy the line from the
original file. If that happens again I’ll use Notepad++ to figure it out. I
think that happened before I was able to coerce Eclipse into always using
tabs.

If you start looking at line endings you will go mad. They are highly
inconsistent in the MH files. I’ve never complained since both windows and
linux ports of Perl seem to handle it all in stride. If we “fix” the line
endings it will forever look like a giant replacement of many files to the
diff tools. It will be difficult to go back and diff old versions of a file
to new ones.

Which specific file / commit is a good example of your issue?

From: krkeegan [mailto:[email protected]]
Sent: Friday, January 18, 2013 12:55 PM
To: hollie/misterhouse
Cc: Michael Stovenour
Subject: Re: [misterhouse] Fix typos in AllLinkDatabase (#41)

So I think we will want to resolve the cause of this, otherwise we may
continue to get spurious "false edits."

Since these particular edits were so small I made them in pico. But I did
not alter the lines in question at all. So whatever is happening
is occurring automatically in pico and is only affecting edits made by
Marc.

I agree that git thinks there is a white space difference, however, I
can't
find one.

I pulled both versions and view them in Notepad++ which will optionally
show whitespaces and tabs. I don't see any difference on these lines, both
files use tabs. I am not aware of one, but are there multiple characters
for tabs similar to newlines?


Reply to this email directly or view it on GitHub <
https://github.com/hollie/misterhouse/pull/41#issuecomment-12435892> .

<
https://github.com/notifications/beacon/J6T91GIPIyhU-8ti4GCGP49g6Yzc-NhLegWj1GR-WW5B_uyeSMdC1Pa7S39S3UlC.gif>


Reply to this email directly or view it on GitHubhttps://github.com//pull/41#issuecomment-12439662.


Reply to this email directly or view it on GitHub #41 (comment) .

https://github.com/notifications/beacon/J6T91GIPIyhU-8ti4GCGP49g6Yzc-NhLegWj1GR-WW5B_uyeSMdC1Pa7S39S3UlC.gif

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants