Repository navigation
TODO/XXX in tools directory #4635
Description
Ref: #264
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
Activity
Fishrock123 commentedon Jan 12, 2016
postinstall doesn't even actually run in the OS X installer (no one knows how pkgmaker actually works)
trendsetter37 commentedon Mar 1, 2016
I can probably start on this. At least with the first one to begin with...
trendsetter37 commentedon Mar 7, 2016
@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.
Conversely, the toplevel operation does not do this so the resulting prefix will only contain
C:as opposed toC:/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.
Fishrock123 commentedon Mar 7, 2016
@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
jasnell commentedon May 30, 2017
@Trott ... any reason to keep this open?
Trott commentedon May 30, 2017
(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.