Review TODOs and FIXMEs

[vkurchatkin]

1. Make an issue for each TODO or FIXME (https://github.com/naholyr/github-todos can help);
2. discuss;
3. PR to fix or remove.

Thoughts?

[julianduque]

👍 I like this approach, would be ideal to leave the TODO or FIXME comment associated with an issue in GitHub as proposed with github-todos, it's sometimes easier to fix or contribute to an issue if you exactly where it is on the code

[rvagg]

I'm not sure about automating this, might be a bit too noisy, but TODOs and FIXMEs are a great place for new contributors to get started and issues could be labelled as such.

[btd]

It could be more easy to automate it in jenkins (as build containers for iojs on jenkins) with task scanner?

[yorkie]

unnecessary a bit, because those comments should be created by some of committers from core team, such that i think those should be personal work rather than community actually, issues or PRs would also absolutely depends on someone who created the TODO :(

[vkurchatkin]

I was talking about one time thing, not about actual hook.

TODOs and FIXMEs are a great place for new contributors to get started and issues could be labelled as such.

Indeed, but comments can be rather vague or obsolete, so real github issues are required.

[yorkie]

haha @vkurchatkin you are right, i'm confused by Rod said automation :)

[bnoordhuis]

I suspect that a sizable fraction of TODOs and FIXMEs are stale or not actionable.

I know I'm guilty of putting TODOs in my code that assume that once we have $mythical_feature_x, we'll finally be able to do this right! I'm probably not the only one who does. :-)

[caitp]

I'm bored, I'll propose a change for the most trivial of these that I can find :)

[rvagg]

haha, very well done @caitp, next!

[piscisaureus]

@caitp

If you want to help out, I'd suggest the following: fix fs error messages.

Currently if fs.rename('foo', 'bar') fails, an error gets printed that looks like this:
Error: ENOENT, rename 'C:\Users\Bert Belder\foo'
The error includes only the first or the second path, depending on what the error was. That makes very little sense. Instead, what I'd like to see is:
Error: ENOENT, rename 'C:\Users\Bert Belder\foo' -> 'C:\Users\Bert Belder\bar'

The same also applies to:

• fs.link(source, target);
• fs.symlink(dest, path, type), but only on windows when type is junction. In other cases only the second path should be part of the error message.

[caitp]

sure, I'll create an issue with an idea I've got there

[piscisaureus]

@caitp See also #207

[mrjoelkemp]

@piscisaureus @caitp is the error message issue still unresolved? I'd like to give it a shot.

I'd also love more beginner-friendly tickets being labeled with help-wanted.

[mrjoelkemp]

Nevermind. I see they've been fixed

[Qard]

I wonder if we should consider making future TODO/FIXME comments include issue numbers?

[Trott]

This issue is a year old and has seen no recent activity.

There might be some imperfections in my methods, but when I search through the current master branch and remove obvious false positives (markdown files, things in deps or tools that we don't control, etc.), here's what I get:

Path	TODOs,FIXMEs,XXXs
configure	2
lib	61
Makefile	1
Makefile.build	1
src	44
test	36
tools	12

I'd like to close this issue and open issues to replace it focusing on particular areas, one each for:

• lib
• src
• test
• tools (just cpplint.py and sx-pkg-postinstall.sh)
• Makefile, Makefile.build, configure

The one for lib in particular will likely need to be broken down further, but this will make it manageable (for the ones other than lib at least) to keep a task list of existing TODO (etc.) items in each area.

I'll wait at least a couple days in case anyone has a strong objection to this approach. (I'm guessing some people will think it's a pointless exercise, but I'm optimistic that no one will think that it's actively harmful.)

[vkurchatkin]

@Trott let's do this!

[bnoordhuis]

@Trott You can drop cpplint.py from the list, it's mostly not written by us.

[thefourtheye]

Actually in few places people's names are also used.

[Trott]

Wow, I had a lot of false positives in the test directory. Turns out there were only 11 in there. Still have to open a PR for src and one for lib and then this can be closed.

[Trott]

OK, separate issues opened. Some of them may yet still need to be broken down further, but it's a start. Closing this one.