Page MenuHomeSoftware Heritage

npm.loader: Extract _author_str function and add tests around
ClosedPublic

Authored by ardumont on Jul 22 2020, 3:19 PM.

Details

Summary

The goal is to fix an issue in a dedicated diff [1]

[1] D3596

Test Plan

tox

Diff Detail

Event Timeline

Build is green

Patch application report for D3595 (id=12650)

Rebasing onto c9b503ed08...

Current branch diff-target is up to date.
Changes applied before test
commit db87f1d415fdec54607fa1db0040c496adf5cd04
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 22 15:18:37 2020 +0200

    npm.loader: Extract _author_str function and add tests around

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/185/ for more details.

Read simplification without typos ;)

Build is green

Patch application report for D3595 (id=12651)

Rebasing onto c9b503ed08...

Current branch diff-target is up to date.
Changes applied before test
commit 6050cdd4a63ffccf785113c7ff0bbf1ab9344bb4
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 22 15:18:37 2020 +0200

    npm.loader: Extract _author_str function and add tests around

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/186/ for more details.

Build is green

Patch application report for D3595 (id=12652)

Rebasing onto c9b503ed08...

Current branch diff-target is up to date.
Changes applied before test
commit fb26841643b5a32cdf8a5d16c896cf6218289ef5
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 22 15:18:37 2020 +0200

    npm.loader: Extract _author_str function and add tests around

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/187/ for more details.

Add missing cases (except the problematic one for another diff)

Build is green

Patch application report for D3595 (id=12653)

Rebasing onto c9b503ed08...

Current branch diff-target is up to date.
Changes applied before test
commit 1abbcfc1ff85b5696865e0a61962a71ebc3aa4ee
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 22 15:18:37 2020 +0200

    npm.loader: Extract _author_str function and add tests around

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/188/ for more details.

Build has FAILED

Patch application report for D3595 (id=12655)

Rebasing onto c9b503ed08...

Current branch diff-target is up to date.
Changes applied before test
commit c4a4ddd37dd1d124d9b4c9f7de300fc0dd482d4e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 22 15:18:37 2020 +0200

    npm.loader: Extract _author_str function and add types and tests

Link to build: https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/190/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/190/console

Build has FAILED

Patch application report for D3595 (id=12655)

Rebasing onto c9b503ed08...

Current branch diff-target is up to date.
Changes applied before test
commit c4a4ddd37dd1d124d9b4c9f7de300fc0dd482d4e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 22 15:18:37 2020 +0200

    npm.loader: Extract _author_str function and add types and tests

Link to build: https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/194/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/194/console

Build has FAILED

Picture me astonished ;)

swh/loader/package/npm/loader.py
190

yeah... that won't work ;)

Fix typo again!

swh/loader/package/npm/loader.py
190

was "email" instead of "name", fixed.

Build is green

Patch application report for D3595 (id=12659)

Rebasing onto c9b503ed08...

Current branch diff-target is up to date.
Changes applied before test
commit 033591dfc3aacb4cc5078fda3ab12170dc231c70
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 22 15:18:37 2020 +0200

    npm.loader: Extract _author_str function and add types and tests

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/195/ for more details.

I assume you changed the behavior of _author_str to deal with present by empty emails. If so, could you add a test for that?

This revision now requires changes to proceed.Jul 23 2020, 9:58 AM

I assume you changed the behavior of _author_str to deal with present by empty emails. If so, could you add a test for that?

No.
Did i change the behavior here (I assumed i did not)?

Former code:

if "email" in author_data:
    author_str += " <%s>" % author_data["email"]

New code:

email = author_data.get("email")
if email is not None:
    author_str += f" <{email}>"

What is the benefit of the new code, if not to handle the case where author_data["email"] is None?

What is the benefit of the new code, if not to handle the case where author_data["email"] is None?

1 access less to author_data...
I did not realize the extra case, thanks.

This diff was just initially about extracting and starting having dedicated tests about it.
I started refactoring it in the next diffs and thought... well it can be done here instead... (to avoid mixing concerns).

I'll had the missing test case.
Thanks.

(fortunately, jenkins is back to "normal" speed now... so that should go fast)

Build is green

Patch application report for D3595 (id=12668)

Rebasing onto c9b503ed08...

Current branch diff-target is up to date.
Changes applied before test
commit 9894b477f5a226b8256f8a2eb56c6695883fdf12
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 22 15:18:37 2020 +0200

    npm.loader: Extract _author_str function + add types, tests

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/199/ for more details.

This revision is now accepted and ready to land.Jul 23 2020, 10:28 AM