Collect NPM #101

[NavonilDas]

Collecting vulnerabilities from npm registry.

[pombredanne]

@NavonilDas thank you ++ for this. This is a very good start!

A few global comments before I can review further:

1. can you ensure you rebase your branch on the latest develop branch?
2. please use pep8 formatting
3. use a docstring for function documentation rather than a # comment before the function
4. most important of all, we need some unit tests for all this

[pombredanne]

Do you mind to sort imports?
Also can you add a standard license header as in the other files?

[pombredanne]

Could you make this a docstring rather than a comment? Also plase make sure you use pep8-like formatting (e.g. 2 empty lines between top level members)

[NavonilDas]

ok, I will change the requested things.

[pombredanne]

Please avoid end of line comments if possible. And use single ' quotes unless for docstrings.

That said, what is the purpose of this function? Are spaces an issue for the semantic version library?
And this is a rather complex function, so we absolutely need many tests for this

[NavonilDas]

This function removes multiple spaces and v character in front of the version (like v2.1.3). The semantic version module throws parsing exception due to these conditions.

yes, tests would be a good idea.

[haikoschol]

@NavonilDas Thanks for making those changes. However, your last commit is missing the "Signed-off-by" bit. Could you please add it? You can automate that for future commits with a git hook:

$ cat .git/hooks/prepare-commit-msg
#!/bin/sh

NAME=$(git config user.name)
EMAIL=$(git config user.email)

if [ -z "$NAME" ]; then
    echo "empty git config user.name"
    exit 1
fi

if [ -z "$EMAIL" ]; then
    echo "empty git config user.email"
    exit 1
fi

git interpret-trailers --if-exists doNothing --trailer \
    "Signed-off-by: $NAME <$EMAIL>" \
    --in-place "$1"

Your branch also still needs to be rebased on current develop. The conflicting code on both branches does the same thing, just with different syntax (string formatting with '{}'.format(...) vs f'{...}'). I prefer the f'{...}' syntax, but pick whichever one you want.

Let me know if you need any help. I can probably also do the rebase for you if you prefer that.

[NavonilDas]

@haikoschol Thanks for the script.

After writing a few Tests I will rebase it to develop branch.

[pombredanne]

Thank you for the update and the progress... See my comments inline.

[pombredanne]

I am not sure why this (which is from #124 ) ends up in your PR.... di you merge rather than rebase on develop?

[pombredanne]

You have an unresolved merge conflict here... but why would your npm code be related to archlinux code?

[haikoschol]

This is because we both made some trivial changes to the same lines of the archlinux code. That's what I was referring to at the end of my earlier comment (#121 (comment)):

The conflicting code on both branches does the same thing, just with different syntax (string formatting with '{}'.format(...) vs f'{...}'). I prefer the f'{...}' syntax, but pick whichever one you want.

[haikoschol]

It looks like all review comments have been addressed. There is only one commit failing the DCO check. Since most of the commits on this branch only contain styling or merge fixes, I'll squash them and merge the PR.