Skip to content

Conversation

@Planeshifter
Copy link
Member

Resolves #60 .

Checklist

Please ensure the following tasks are completed before submitting this pull request.

  • Read, understood, and followed the contributing guidelines, including the relevant style guides.
  • Read and understand the Code of Conduct.
  • Read and understood the licensing terms.
  • Searched for existing issues and pull requests before submitting this pull request.
  • Filed an issue (or an issue already existed) prior to submitting this pull request.
  • Rebased onto latest develop.
  • Submitted against develop branch.

Description

What is the purpose of this pull request?

This pull request:

Related Issues

Does this pull request have any related issues?

This pull request:

  • resolves #
  • fixes #

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.


@stdlib-js/reviewers

@Planeshifter Planeshifter requested a review from kgryte December 12, 2016 23:31
## Usage

``` javascript
var homedir = require( '@stdlib/lib/node_modules/@stdlib/utils/homedir' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The require statement is incorrect.


If unable to locate a `home` directory, the module returns `null`.

``` javascript
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure we should include this example. Could cause issues if we ever perform automated testing of doc examples.


## Notes

* This module primarily checks various [environment variables][environment-variables] to locate a `home` directory. Note that this approach has __security vulnerabilities__, as attackers can tamper with [environment variables][environment-variables].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation primarily checks...

## Examples

``` javascript
var homedir = require( '@stdlib/lib/node_modules/@stdlib/utils/homedir' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Require statement is incorrect.

t.end();
});

tape( 'the function returns the `/root` directory if run as `root` in a linux environment', function test( t ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All instances of linux => Linux in string descriptions.

t.end();
});

tape( 'the function returns a home directory in a Mac OSX environment (USERNAME)', function test( t ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All instances of OSX => OS X in string descriptions.

// Get the current user account (https://docs.python.org/2/library/getpass.html):
user = env[ 'LOGNAME' ] || env[ 'USER' ] || env[ 'LNAME' ] || env[ 'USERNAME' ];

// If on Mac OSX, use the Mac path convention (http://apple.stackexchange.com/questions/119230/what-is-standard-for-os-x-filesystem-e-g-opt-vs-usr)...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OS X

</section>

<!-- /.examples -->

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing CLI docs.

// e.g., returns '/Users/<username>'
```

If unable to locate a `home` directory, the module returns `null`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...the function returns null.

@kgryte kgryte added Feature Issue or pull request for adding a new feature. Utilities Issue or pull request concerning general utilities. labels Dec 13, 2016
@Planeshifter
Copy link
Member Author

Could you take another look?

<!-- /.references -->


---
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLI docs go above the "references" section.

t.end();
});

tape( 'the function supports older Node versions', function test( t ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this test whether the implementation supports older Node versions? Considering how this package works, this should be in the other test file.

Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to confirm test works as intended.


opts = {
'os': {
'homedir': void 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm that this works as intended? Possible that proxyquire ignores homedir when set to undefined, falling back to the original implementation. If so, can set to false.

This should be apparent when generating a coverage report and whether all code branches are covered (notably within the main index.js file).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, can confirm that we reach 100% code coverage for homedir.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Including branch coverage?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must be because os does get mocked with an empty object. Makes sense.

@kgryte kgryte merged commit 1a37da4 into develop Dec 13, 2016
@kgryte kgryte deleted the feature/homedir branch December 13, 2016 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature Issue or pull request for adding a new feature. Utilities Issue or pull request concerning general utilities.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement homedir

3 participants