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

Adding headers of the deltacode output in the cli after writing to the json file is over #134

Closed
wants to merge 1 commit into from

Conversation

Pratikrocks
Copy link
Collaborator

@Pratikrocks Pratikrocks commented Mar 12, 2020

Signed off by: Pratik Dey [email protected]
referencing to issue #130
added headers of the deltacode output to the cli.

@Pratikrocks
Copy link
Collaborator Author

@MaJuRG @pombredanne Please kindly review my PR .:)

configure Outdated
@@ -27,7 +27,7 @@ CONFIGURE_ROOT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
if [[ "$PYTHON_EXE" == "" ]]; then
PYTHON_EXE=python
fi

PYTHON_EXE=python2.7
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually I had 3 versions of python in my system to explicitly configure with py2.7 I was using it,else it was showing config errors(I am going to change it now)

click.echo('deltacode_options: {')
for option in deltacode.options:
click.echo(""" "{}": "{}" """.format(option,deltacode.options[option]))
click.echo("}")
click.echo("""deltacode_version: "{}" """.format(__version__))
click.echo('deltacode_errors: {}'.format(collect_errors(deltacode)))
click.echo('deltas_count: {}'.format(len([d for d in deltas(deltacode, all_delta_types)])))
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not really care to see these in the output. #130 is relating to just the Number of Deltas and what the different factors are.

I would like to see the counts of each particular Delta type (factors), not much else.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay @MaJuRG sir so I am updating this part

@Pratikrocks
Copy link
Collaborator Author

@MaJuRG sir I updated it now
Screenshot from 2020-03-13 22-29-41

Comment on lines 54 to 65
click.secho(get_notice(),fg="green")
click.echo('deltas_count: {}'.format(len([d for d in deltas(deltacode, all_delta_types)])))
click.echo('delta_stats: {')
for stat in deltacode.stats.to_dict():
click.echo(""" "{}": {}""".format(str(stat),deltacode.stats.to_dict()[stat]))
click.echo("}")
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think we need to struture the output like a json file. It adds a lot of noise. I would rather it just be output as key: value and not have JSON brackets or indentation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@MaJuRG sir , updated the content

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Screenshot from 2020-03-14 00-49-12

Copy link
Contributor

@steven-esser steven-esser left a comment

Choose a reason for hiding this comment

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

You did not sign off your last commit.

Also, you should improve your commit messages. A commit message of "Updates" tells me nothing of what has been done. See: https://chris.beams.io/posts/git-commit/

Please rebase your "updates" commits into a single commit and update the commit message to something more detailed using the link above.

@Pratikrocks
Copy link
Collaborator Author

@MaJuRG sir, sorry for the delay and inconvenience ,I have made the changes

configure Outdated
@@ -27,7 +27,6 @@ CONFIGURE_ROOT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
if [[ "$PYTHON_EXE" == "" ]]; then
PYTHON_EXE=python
fi

Copy link
Contributor

Choose a reason for hiding this comment

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

Re-add this line break

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@MaJuRG added the line break

@Pratikrocks Pratikrocks force-pushed the addingCLI branch 2 times, most recently from 5cbcf7a to e0f6000 Compare March 15, 2020 15:47
click.echo('deltas_count: {}'.format(len([d for d in deltas(deltacode, all_delta_types)])))
click.echo('delta_stats: ')
for stat in deltacode.stats.to_dict():
click.echo(""" "{}": {}""".format(str(stat),deltacode.stats.to_dict()[stat]))
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think we want these unnecessary spaces in the output. Also, I would like the "stats" to be broken down by "factors" as this is more detailed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@MaJuRG ,I was actually providing the old_files_count ,files_modifies,files_moves....
in the stats part
What else should we provide ?
should we also provide the stats of new and old file path as it appears in the headers of the delta code json file?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like the "stats" to be broken down by "factors" as this is more detailed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes we can provide the factors of each and every deltas in the cli ,and that is actually much elaborate ,however some deltas may be having empty arrays as its "factors" (in case of "added"...) ,what do we need to provide as "factors" in such a case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Then you just use the base status value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes @MaJuRG I think we can provide both the status and factors for the deltas as that will actually much detailed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@MaJuRG can you please review the changes.

@Pratikrocks
Copy link
Collaborator Author

@MaJuRG kindly review this, I made few changes

Screenshot from 2020-03-23 02-01-23

Now I provided the the deltas which are modified in the CLI (deltas score,factors,old file ,new file...),so this is actually providing a bit more details to the end user about the deltas

@Pratikrocks
Copy link
Collaborator Author

@MaJuRG can you review the changes

Copy link
Contributor

@steven-esser steven-esser left a comment

Choose a reason for hiding this comment

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

Can you show me what this looks like in the output?

Comment on lines 267 to 284
def get_notice_for_cli():
"""
Retrieve the notice text from the NOTICE file for display in the Command Line .
"""
notice_path = os.path.join(os.path.abspath(os.path.dirname(__file__)), 'NOTICE')
notice_text = open(notice_path).read()

delimiter = '\n\n\n'
[notice_text, extra_notice_text] = notice_text.split(delimiter, 1)
extra_notice_text = delimiter + extra_notice_text

delimiter = '\n\n '
[notice_text, acknowledgment_text] = notice_text.split(delimiter, 1)
acknowledgment_text = delimiter + acknowledgment_text

notice = acknowledgment_text.strip().replace('\n', '')

return notice
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@MaJuRG the screen shot which I provided it here is the output of this part (the green area)

@MaJuRG kindly review this, I made few changes

Screenshot from 2020-03-23 02-01-23

Now I provided the the deltas which are modified in the CLI (deltas score,factors,old file ,new file...),so this is actually providing a bit more details to the end user about the deltas

Copy link
Collaborator Author

@Pratikrocks Pratikrocks Apr 2, 2020

Choose a reason for hiding this comment

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

@MaJuRG sir I updated it now
Screenshot from 2020-03-13 22-29-41

@MaJuRG now this is the output(for the notice part) when I do not use this additonal function

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why do we need this?

It is added to make the notice output in the CLI look better

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not want the notice displayed in the section.

@Pratikrocks
Copy link
Collaborator Author

@MaJuRG do I need to make some other changes in the CLI output part?

@Pratikrocks Pratikrocks force-pushed the addingCLI branch 2 times, most recently from a443d2d to d9973f8 Compare April 13, 2020 14:39
@Pratikrocks
Copy link
Collaborator Author

@MaJuRG can you please review it again, I have some changes
when we are generating deltas with options
deltacode -n tests/data/cli/scan_1_file_moved_new.json -o tests/data/cli/scan_1_file_moved_old.json -j - , then there is no need to provide a CLI , as the entire output is displayed in the CLI.

Screenshot from 2020-04-13 20-11-42

Now when we are printing the delta outputs in a json file then , I am providing a CLI , after the delta comparisons are required.
Screenshot from 2020-04-13 20-00-38

I felt that it would be better not to have an additional CLI in the first case.


simplejson.dump(results, outfile, iterable_as_array=True, indent=2)
outfile.write('\n')
if outfile.name != '<stdout>' :
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This occours when the -j option is in the json file

Comment on lines 267 to 284
def get_notice_for_cli():
"""
Retrieve the notice text from the NOTICE file for display in the Command Line .
"""
notice_path = os.path.join(os.path.abspath(os.path.dirname(__file__)), 'NOTICE')
notice_text = open(notice_path).read()

delimiter = '\n\n\n'
[notice_text, extra_notice_text] = notice_text.split(delimiter, 1)
extra_notice_text = delimiter + extra_notice_text

delimiter = '\n\n '
[notice_text, acknowledgment_text] = notice_text.split(delimiter, 1)
acknowledgment_text = delimiter + acknowledgment_text

notice = acknowledgment_text.strip().replace('\n', '')

return notice
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not want the notice displayed in the section.

Comment on lines 75 to 77
click.secho("#" * 211 + "\n",fg="yellow")
click.secho(" "*70 + "All deltas are generated, view the json file for details" + "\n" , fg="green")
click.secho("#" * 211,fg="yellow")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this. I dont really care for this output.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@MaJuRG , I will be removing the notice and this additional output sections

@Pratikrocks Pratikrocks force-pushed the addingCLI branch 3 times, most recently from 41fa884 to 85f7324 Compare April 15, 2020 13:29
@Pratikrocks
Copy link
Collaborator Author

Pratikrocks commented Apr 15, 2020

Screenshot from 2020-04-15 15-01-30

@MaJuRG , I made the changes in the output of the CLI part.
Now the output will appear as per the above screen shot.
So, in the output I provided the following things:
1.delta counts
2.delta statistics
3.For each deltas I provided the following info:
status , factors , delta score, original path of new file,original path of old file.

Please suggest if some additional details are required :)

Signed-off-by: Pratikrocks <[email protected]>
@Pratikrocks
Copy link
Collaborator Author

Screenshot from 2020-04-17 16-53-48
@MaJuRG , this is the output after making the recommended changes.

@Pratikrocks
Copy link
Collaborator Author

Pratikrocks commented Apr 20, 2020

Regarding the number of lines changed ,it is more because I rebased the this branch to the tip of the develop branch, so as to reflect the changes here. I only changed the init.py and cli.py rest files are unaltered,The rest changes are present even in the develop branches even

@steven-esser
Copy link
Contributor

Closing due to stagnation.

@Pratikrocks
Copy link
Collaborator Author

@MaJuRG sir I request review on it and all other or of mine a long back, and for many times I had asked
Even my last comment in all other PR was also for review
I was active throughout , but I never used to get reply satisfactoryl.

@steven-esser
Copy link
Contributor

@Pratikrocks Yes this may have fallen through the cracks, sorry about this lack of feedback.

The deltacode repo has been updated since this PR was made with a number of configuration changes. If you can, please rebase your branch, or add your changes to a new branch off of the current develop HEAD. I will make sure to leave prompt feedback if you do!

@Pratikrocks
Copy link
Collaborator Author

Pratikrocks commented Mar 19, 2021

Hi @MaJuRG , hope you are well,yes there are many changes currently :)
I see now that now deltacode is already ported to py3
But we are not having the support for VirtualCodeBase thing from ScanCode right?
So do you want deltacode to have the support for the VirtualCodeBase , if so I will make a separate PR for it :)?

@steven-esser
Copy link
Contributor

Yes, deltacode is now python3.

And yes, we are still looking to support VirtualCodebase, which has not been completed yet. PRs welcome 👍

@Pratikrocks
Copy link
Collaborator Author

Okay

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.

2 participants