Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix to #27211 - Refactor RealEnvStore methods to use libuv methods for env operations #27310

Closed
wants to merge 10 commits into from
Closed

Fix to #27211 - Refactor RealEnvStore methods to use libuv methods for env operations #27310

wants to merge 10 commits into from

Commits on Aug 20, 2019

  1. src: modified RealEnvStore::Get method to use libuv functions 

    Modified RealEnvStore::Get - removed os switch statements
and replaced logic to use libuv uv_getos_env method.

Fixes: nodejs#27211
Refs: http://docs.libuv.org/en/v1.x/misc.html
    @devasci @addaleax
    devasci authored and addaleax committed Aug 20, 2019
    Configuration menu
    Copy the full SHA
    1501433 View commit details
    Browse the repository at this point in the history

  2. src: modified RealEnvStore methods to use libuv functions 

    Modified RealEnvStore::Get, Set, Query and Delete methods
to use libuv methods environment variables operations instead
of using os specific logic and switches.

Fixes: nodejs#27211
Refs: http://docs.libuv.org/en/v1.x/misc.html
    @devasci @addaleax
    devasci authored and addaleax committed Aug 20, 2019
    Configuration menu
    Copy the full SHA
    eef9b8b View commit details
    Browse the repository at this point in the history

  3. src: Modified src/node_env_var.cc as per Joyee Cheung’s review comments 

    Below review comments are taken care in this submission:
1. followed snake case naming convention for local variables
2. dropped sizeof(char) since it is obvious value 1
3. Used MaybeLocal<String> instead of Local<String> to check
for
   empty strings and throw exception if empty.

Fixes: nodejs#27211
Refs: https://v8docs.nodesource.com/node-4.8/annotated.html
    @devasci @addaleax
    devasci authored and addaleax committed Aug 20, 2019
    Configuration menu
    Copy the full SHA
    7db9f03 View commit details
    Browse the repository at this point in the history

  4. src: Modified to use 2 space for indentation. 

    used 2 spaces instead of 4 for indentation

Fixes: nodejs#27211
Refs: https://v8docs.nodesource.com/node-4.8/annotated.html
    @devasci @addaleax
    devasci authored and addaleax committed Aug 20, 2019
    Configuration menu
    Copy the full SHA
    6ae79f7 View commit details
    Browse the repository at this point in the history

  5. src: refactor RealEnvStore methods - review comments fixing - 1 

    Below review comments by Anna Henningsen are taken care:
	1. avoided Yoda style comparisons
	2. used MaybeStackBuffer instead of raw char pointers
	3. used MaybeLocal<String> to inspect for empty string value
            and then raise exception and return empty Local<String> handle.
            (Changing return type of RealEnvStore::Get method to MaybeLocal<String>
              is pending, and planning to submit in next commit)

Fixes: nodejs#27211
Refs: nodejs#27310 (comment)
    @devasci @addaleax
    devasci authored and addaleax committed Aug 20, 2019
    Configuration menu
    Copy the full SHA
    0271ee1 View commit details
    Browse the repository at this point in the history

  6. src: refactor RealEnvStore methods - review comments fixing - 2 

    Modified KVStore::Get return type to MaybeLocal and also modified
RealEnvStore::Get, MapKVStore::Get respectively.

Fixes: nodejs#27211
Refs: nodejs#27310 (comment)
    @devasci @addaleax
    devasci authored and addaleax committed Aug 20, 2019
    Configuration menu
    Copy the full SHA
    4d51c93 View commit details
    Browse the repository at this point in the history

  7. src: refactor RealEnvStore methods - review comments fixing - 2a 

    Removed exception throwing if value is empty, since the same is handled
in caller and returned immediately.

Fixes: nodejs#27211
Refs: nodejs#27310 (comment)
    @devasci @addaleax
    devasci authored and addaleax committed Aug 20, 2019
    Configuration menu
    Copy the full SHA
    1d0aa6e View commit details
    Browse the repository at this point in the history

  8. src: refactor RealEnvStore methods - review comments fixing - 3 

    1. All tests were failing due to the exception raising from within 

   RealEnvStore::Get method, removed the exception raising since 

   the caller are checking for empty handles and taking necessary 
actions.
2. Used MayLocal<String>’s ToLocalChecked() function instead of
FromMaybe().

Fixes: nodejs#27211
Refs: nodejs#27310 (comment)
    @devasci @addaleax
    devasci authored and addaleax committed Aug 20, 2019
    Configuration menu
    Copy the full SHA
    b40d71e View commit details
    Browse the repository at this point in the history

  9. src: refactor RealEnvStore methods - Joyee Cheung’s review remarks fixed 

    Modified an if statment and formatted the code as per Joyee Cheung's
review comments

Fixes: nodejs#27211
Refs: nodejs#27310 (comment)
Refs: nodejs#27310 (comment)
    @devasci @addaleax
    devasci authored and addaleax committed Aug 20, 2019
    Configuration menu
    Copy the full SHA
    88fedfc View commit details
    Browse the repository at this point in the history

  10. src: handle windows hidden/read-only env keys in set/query methods 

    All windows tests were failing after previous commit, as per
Joyee Cheung suggestion modified to inspect the first character
of env variable key’s on windows machine for the character ‘=‘
and skip if the key contains. Since these keys are hidden/read-only
env vars in windows machines.

Fixes: nodejs#27211
Refs: nodejs#27310 (comment)
    @devasci @addaleax
    devasci authored and addaleax committed Aug 20, 2019
    Configuration menu
    Copy the full SHA
    614f357 View commit details
    Browse the repository at this point in the history