TODO/XXX in tools directory

@bnoordhuis

There are two TODO comments and one XXX comment in the tools directory that were placed there by project authors (as opposed to pre-existing in tools imported from outside the project). It would be great to either remove them from the code (if they are no longer valid or at least not particularly high value), or get issues opened for them, or just get whatever it is they are addressing addressed. Here they are as of this writing.

1: cpplint.py:

fullname = self.FullName()
# XXX(bnoordhuis) Expects that cpplint.py lives in the tools/ directory.
toplevel = os.path.abspath(os.path.join(os.path.dirname(__file__), '..'))
prefix = os.path.commonprefix([fullname, toplevel])
return fullname[len(prefix) + 1:]

There are other comments TODO etc. comments in cpplint.py but they were already there when the external project was first imported into the Node.js project.

2 and 3: osx-pkg-postinstall.sh

#!/bin/sh
# TODO Can this be done inside the .pmdoc?
# TODO Can we extract $PREFIX from the installer?
cd /usr/local/bin
ln -sf ../lib/node_modules/npm/bin/npm-cli.js npm

cc @bnoordhuis

2 and 3: osx-pkg-postinstall.sh

postinstall doesn't even actually run in the OS X installer (no one knows how pkgmaker actually works)

I can probably start on this. At least with the first one to begin with...

@Trott

@Trott @Fishrock123 Regarding number one, I saw that the expected prefix is not removed if a user is on a Windows machine. This happens because the self.Fullname() converts backslashes to forward slashes.

def FullName(self):
    """Make Windows paths like Unix."""
    return os.path.abspath(self._filename).replace('\\', '/')

Conversely, the toplevel operation does not do this so the resulting prefix will only contain C: as opposed to C:/Code/node/.

The difference in return values will be Code/node/tools/cpplint.py & tools/cpplint.py. I think the latter is the desired behavior. Is this going in the right direction @Fishrock123? If so I can go ahead and send a PR with a small fix that consists of converting the backslashes in the toplevel var along with removing the comment.

Note that the Code directory is just where my node repo lives.

@trendsetter37

@trendsetter37 That seems about right. As a note, IIRC we don't actually run cpplint on Windows, so it could be a step to resolving that.

ref issue nodejs#4635

@Trott

@Trott ... any reason to keep this open?

@jasnell

(copy/pasted from a related issue)

@jasnell I'm OK with closing this and the other TODO/XXX/FIXME issues. If any of those items are things that really ought to be fixed (rather than a wishlist or a "will fix after Magical Feature X is available"), a separate issue should be opened anyway because it's just getting lost in these out-of-date tracking issues.

While I think this issue is superfluous personally, anyone else should feel free to re-open (if GitHub permits them to) or comment requesting this be re-opened.

Navigation Menu

Repository navigation

TODO/XXX in tools directory #4635

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

TODO/XXX in tools directory #4635

Description

Activity

Fishrock123 commented on Jan 12, 2016

trendsetter37 commented on Mar 1, 2016

trendsetter37 commented on Mar 7, 2016

Fishrock123 commented on Mar 7, 2016

jasnell commented on May 30, 2017

Trott commented on May 30, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions