Code cleanup proposal: blank lines that are really tabs, names of variables, missing headers, removing duplicates and others

[josephsl]

Hi,

This was brought up by @derekriemer a few months ago somewhere (the dev list, Twitter, IRC, etc.):

When we look at NVDA source code, it appears to be a spider web of different coding styles that contribute to the mess we're seeing today. Some of the issues found were missing headers, blank lines that are really tabs, inconsistent variable names and others.

Thus I would like to propose that we spend at least two months just working on making NVDA source code look polished. In other words, I'd like to suggest that we look at files that constitute NVDA Core and make source code more consistent. This includes tackling the following issues:

• Locating tabbed blank lines.
• Removing duplicates.
• Adding headers.
• Tiding up names of variables, class inheritance declarations and so on.

These are just some of the things we can see in our source code (I'm sure there are more). As this is a huge project, I'd like to request many to help out with this task, with folks working on specific areas such as locating duplicates (I heard @derekriemer is interested in adding copyright headers and making source code docs clearer).

Thanks.

[josephsl]

Hi,

We should also take this time to truly deprecate code paths that ought to leave this world (for instance, config.save). Thanks.

[LeonarddeR]

I'd be happy to contribute to this, so feel free to assign parts of this work to me. I have no idea where to start, though. Sent on behalf of @BabbageCom

[jcsteh]

While the issues here are certainly valid, I don't think they should be addressed in a broad issue or PR like this. Massive changes like this in one go tend to result in a lot of merge conflicts and are an absolute nightmare to review. In addition, beyond changes which can be applied automatically, every change needs to be tested for unexpected effects. We just do not have the resources to review and do QA for a massive project like this. I think it is better to address issues like these in smaller chunks; e.g. while working on that area of the code.

[josephsl]

Hi, agreed about the issue management. I’m sure this one could be the start of a discussion (via a series of issues) where folks can post issues with code as they find it (separate issues). Thanks. From: James Teh [mailto:[email protected]] Sent: Wednesday, November 23, 2016 11:45 PM To: nvaccess/nvda <[email protected]> Cc: Joseph Lee <[email protected]>; Author <[email protected]> Subject: Re: [nvaccess/nvda] Code cleanup proposal: blank lines that are really tabs, names of variables, missing headers, removing duplicates and others (#6589) While the issues here are certainly valid, I don't think they should be addressed in a broad issue or PR like this. Massive changes like this in one go tend to result in a lot of merge conflicts and are an absolute nightmare to review. In addition, beyond changes which can be applied automatically, every change needs to be tested for unexpected effects. We just do not have the resources to review and do QA for a massive project like this. I think it is better to address issues like these in smaller chunks; e.g. while working on that area of the code. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#6589 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgLkB_G22A_eSz5MuZJy5Uw_Qdu_8jMks5rBUB_gaJpZM4K7WDP> .

[jcsteh]

I don't follow the purpose of this issue, though. It's a task which can never be said to be complete because of it's bredth. It is ongoing work; there's no defined beginning or ending.

[Brian1Gaff]

Although I do understand the need for this for the future, it is very very easy to screw something up accidentally here as the person doing it is not likely to be the person responsible for the routine in the first place. I can remember when I was younger and was running a project written in Assembler, lots of extra bugs got added by a new helpful person. I'm past all that these days, but I just thought I'd mention it as a danger. You also need to police it as many people might be working on locally stored source code and undo what was fixed a while before! Brian [email protected] Sent via blueyonder. Please address personal email to:- [email protected], putting 'Brian Gaff' in the display name field. ----- Original Message ----- From: "Joseph Lee" <[email protected]> To: "nvaccess/nvda" <[email protected]> Sent: Thursday, November 24, 2016 7:18 AM Subject: [nvaccess/nvda] Code cleanup proposal: blank lines that are really tabs, names of variables, missing headers, removing duplicates and others (#6589)

…

Hi, This was brought up by @derekriemer a few months ago somewhere (the dev list, Twitter, IRC, etc.): When we look at NVDA source code, it appears to be a spider web of different coding styles that contribute to the mess we're seeing today. Some of the issues found were missing headers, blank lines that are really tabs, inconsistent variable names and others. Thus I would like to propose that we spend at least two months just working on making NVDA source code look polished. In other words, I'd like to suggest that we look at files that constitute NVDA Core and make source code more consistent. This includes tackling the following issues: * Locating tabbed blank lines. * Removing duplicates. * Adding headers. * Tiding up names of variables, class inheritance declarations and so on. These are just some of the things we can see in our source code (I'm sure there are more). As this is a huge project, I'd like to request many to help out with this task, with folks working on specific areas such as locating duplicates (I heard @derekriemer is interested in adding copyright headers and making source code docs clearer). Thanks. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #6589

[tsiegel]

I don't know why blank lines that are really tabs is an issue, but those kinds of things are easily solved. A simple program that looks for lines with only tabs in it, and replacing them with cr/crlf pairs is trivially easy to write, and could be run on all of the source tree in a matter of seconds. No need for hand checking. Copyright headers for all files could be done in a similar manner.

[jcsteh]

Copyright headers can't be automated for several reasons. Some commits can't be considered to affect copyright; e.g. a commit which just changes whitespace or something equally trivial should not be considered as a copyright holder. Some commits were made on behalf of others and that information is in the commit message itself, usually because git doesn't support multiple commit authors itself. Some commits have incorrect user details and would need remapping. Of course, if we have a list of those, we can map them, but that The more time we spend reviewing trivial (but very time consuming) stuff like this, the less time we have to review real changes.still requires manual effort. Even if this is partly automated, it still needs to be manually reviewed and that is the piece of work that will be hugely time consuming.

[tsiegel]

I wasn't talking about copyright for each and every commit, I was talking about general copyright messages for each file. Each file should have the same copyright message stating the overall copyright for the project as a whole. Individual commiters can add their own copyright or not, but generally, for large projects, individual commits/individuals don't get their own copyright line, as they donate their code to the project as a whole.

[jcsteh]

That's how we used to do it, but the problem is that it makes it difficult to determine the copyright of given pieces of code. This might be necessary in the event that we want to contact all copyright holders for a given area of the code, for example. So now, copyright headers should contain all authors who hold copyright for that file.

[feerrenrut]

While it would be nice to have a very clean, tidy and consistent codebase I dont think this is a good idea. Its a moving target, which does not introduce benefit to end users. From my experience these kinds of changes only introduce problems for little benefit.

I vote that we close this issue.

[jcsteh]

I agree with closing this. However, it should be noted that isolated cleanup while working on a particular area of code is okay. For example, if you're making significant changes to a function anyway, it's probably reasonable to tidy up inconsistencies, etc.

[tsiegel]

Just one final comment on this as regards copyright. This is a GPL project, it should be under a copyleft license, and when someone donates code to the project, it should be understood that they may or may not be referenced in the general documentation as a contributor, but having dozens of copyrights for a single source file is just silly, and no other gpl project I've ever seen does this. Generally, it's understood by the contributors that copyright isn't an issue when contributing to an opensource GPL copyleft project. Those who have a problem with this sort of a policy need not contribute code to the project. I've seen serious problems arrise from trying to keep separate copyrights on code, and in one case, it forced the company to change their entire website and snippets collection to have a blanket mit license on all code posted on the site. Let's not open any sort of can of worms here, and just declare all code submitted to NVDA to be a simple copyleft license, and leave it at that.

[feerrenrut]

I'm going to close this issue. However if working on an area of code, feel free to take the extra step to clean it up.