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.

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

Add new duration field to JSON output #1937 #1942

Merged
merged 1 commit into from
Mar 5, 2020
Merged

Add new duration field to JSON output #1937 #1942

merged 1 commit into from
Mar 5, 2020

Conversation

MankaranSingh
Copy link
Contributor

Fixes #1937

Tasks

  • Reviewed contribution guidelines
  • PR is descriptively titled 📑 and links the original issue above 🔗
  • Tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR
    Run tests locally to check for errors.
  • Commits are in uniquely-named feature branch and has no merge conflicts 📁

@MankaranSingh
Copy link
Contributor Author

MankaranSingh commented Feb 26, 2020

@pombredanne Sorry for inconvenience, but i thought it was better to open a new clean pull request instead of fixing #1939
I have made all required changes ! thanks for your guidance !

Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

Thank you! we are getting there...
See one comment inline.
We also need a few extra things:

  1. add at least one test
  2. add your name to the authors and changelog
  3. squash this in one commit and improve your commit message (See https://aboutcode.readthedocs.io/en/latest/aboutcode-docs/writing_good_commit_messages.html for help)

@@ -949,8 +949,11 @@ def echo_func(*_args, **_kwargs):
codebase.counters['final:files_count'] = files_count
codebase.counters['final:dirs_count'] = dirs_count
codebase.counters['final:size_count'] = size_count

processing_end = time()
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need an intermediate variable?
May be you could directly use time() in https://github.com/nexB/scancode-toolkit/pull/1942/files#diff-1e37ecfb9a8c3fccd8ecdfb2ce51f591R956 ?

@codecov
Copy link

codecov bot commented Feb 27, 2020

Codecov Report

Merging #1942 into develop will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1942      +/-   ##
===========================================
+ Coverage    78.93%   79.00%   +0.06%     
===========================================
  Files          131      131              
  Lines        16946    16948       +2     
===========================================
+ Hits         13376    13389      +13     
+ Misses        3570     3559      -11     
Impacted Files Coverage Δ
src/scancode/cli.py 76.99% <0.00%> (+0.16%) ⬆️
src/licensedcode/match.py 78.67% <0.00%> (+0.36%) ⬆️
src/commoncode/command.py 86.61% <0.00%> (+0.70%) ⬆️
src/typecode/magic2.py 93.69% <0.00%> (+1.80%) ⬆️
src/scancode/api.py 96.17% <0.00%> (+2.54%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 850399b...4a91b8f. Read the comment docs.

@MankaranSingh
Copy link
Contributor Author

@pombredanne i did the required changes, hope they are ok :)
also sorry for delays as my exams are going on and i get free only around this time of the night.

Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

Thank you for addressing all my comments! I have a few extra ones inline and here.
For the commit message you could try this: (note the first line is less than 50 chars and note the use of the imperative style)

Add new duration field to JSON output #1937 

This field contains the scan duration in seconds. 

Reported-by: Armijn Hemel @armijnhemel
Signed-off-by: MankaranSingh <42907308+MankaranSingh@users.noreply.github.com>

CHANGELOG.rst Outdated Show resolved Hide resolved
@@ -1,4 +1,4 @@
#
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Must had been a typo.

with open(result_file) as result:
headers = json.loads(result.read())['headers']
assert headers[0]['duration']
assert headers[0]['duration'] >= 0
Copy link
Member

Choose a reason for hiding this comment

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

Excellent!

@pombredanne pombredanne changed the title 'duration' field added to output JSON Add new duration field to JSON output #1937 Feb 27, 2020
This field contains the scan duration in seconds.

Reported-by: Armijn Hemel @armijnhemel
Signed-off-by: MankaranSingh <42907308+MankaranSingh@users.noreply.github.com>
@MankaranSingh
Copy link
Contributor Author

@pombredanne all the required changes done :)

Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

All good!
Thank you++

@pombredanne pombredanne merged commit 17067e2 into aboutcode-org:develop Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add duration of the scan to JSON
2 participants