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

CLI Options Reference for v3.1.1 #27

Merged
merged 24 commits into from
Oct 22, 2019

Conversation

AyanSinhaMahapatra
Copy link
Member

This PR adds to Scancode-Toolkit documentation the Command Line Interface Options reference pages.

Documentation Style is enforced by using Doc8 and Doc Contribution Guide is updated accordingly.
300+ Style errors fixed.

Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
This commit adds to the Scancode Version 3.1.1 doc branch -> 2 main sample tutorials, How to Run a Scan, and
How to Visualize Scan Results. It also makes changes to How to Format Scan output and How to Set what will be
Detected in a Scan to be compatible with Scancode version 3.1.1, and adds images/gifs (May be updated later).
Also adds minor links/referances.

Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
This commit Adds minor changes to Doc Version 3.1.1 which doesn't belong to CLI options or
the Basic Tutorials. CLI options are to be added next.

Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
This commit Adds to the Scancode Doc 3.1.1 rst-snippets, which contains major sections in CLI options,
to be included in list-options.rst(also added in this commit) and other major CLI Options pages.

Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
This commit adds to Scancode Doc version 3.1.1 a Synopsis page, Getting Help From Command Line Page,
Includes the How to Perform a scan Tutorial as an example, and adds Gifs/Images (some outdated ones
will be replaced later).

Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
This commit adds to the Scancode Version 3.1.1 docs, A page detailing all the output formats.
Also modifies rst_snippets related to the custom_output format case.
(An Error present, marked [ToDo], will be solved)

Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
This commit adds to Scancode Doc Version 3.1.1, pages for  Core Options, Output Filters and OutputControl,
Post Scan Options, and Pre Scan Options Pages.

Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
This commit adds minor changes to contrib_doc.rst, and at cli-referance/, adds changes to
synopsis, index and help-text-options.

Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
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! I added a first set of review comments for your consideration.

`Core` Options
==============

.. include:: /scancode-toolkit/rst_snippets/basic_options.rst
Copy link
Member

Choose a reason for hiding this comment

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

Is it normal to have absolute slash-prefixed paths there?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not. Even doc8 gives an error when checking for doc standards here. I thought it'd be better if all the snippets are in one location. If you prefer otherwise I'll change that.
[Note]
Some snippets are used cross-section but a majority of them are not. For example,
rst_snippets/custom_output_format.rst is included both in cli-referance/output-format.rst and tutorials/how_to_format_scan_output.rst. What should I do in these cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there is a problem with the location of the snippets, per-say. I think @pombredanne just means we should use relative paths if possible? Not sure how sphinx is configured in this case, but changing to ./scancode-toolkit/rst_snippets/basic_options.rst might be enough here?

Copy link
Member Author

Choose a reason for hiding this comment

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

That'd give an error InputError: [Errno 2] No such file or directory: 'source/scancode-toolkit/cli-reference/scancode-toolkit/rst_snippets/post_scan_options.rst'. as in sphinx using ./ relates to the rst_snippets being located in the same directory as the file it's being included in. Right?


.. include:: /scancode-toolkit/rst_snippets/core_options.rst

``--generated`` Options
Copy link
Member

Choose a reason for hiding this comment

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

That's great doc, but as I am reading this I wonder if this should not be included in the --info option rather than be a separate option. (this is just a remark... but your opinion is welcomed as a future evolution of the scancode CLI user experience)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an interesting point. Most other options are in some way dependent on other options, (You've also asked -> Should not --max-email be side-by-side with the -e option?) like the --summary option is dependent on --info and has some dependent options of its own. Rather than "including" them in each other, I think they should be kept in separate sections as it is presently. What can be done to better show these dependencies is I could add Notes on every option who they are dependent on and what other options are dependent.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pombredanne it probably should be included in the --info, but this is a scancode-toolkit specific issue I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another question, looks like --generated is independent of --info, and even without -i it works normally without errors. Should this dependency be pointed out?
In other cases, I'm going with notes like :

The option --max-email is a sub-option of and requires the option(s) --email

./scancode -clpieu --json-pp sample_generated.json samples --generated

In the results, for each file the following attribute is added with it's corresponding True/False
value ::
Copy link
Member

Choose a reason for hiding this comment

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

May be True/False should be lowercase since the JSON output is true/false?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Corrected this. Should I push a single commit with all requested changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

This is updated.


"is_generated": true

For the samples folder, the following files have a true value for their is_generated attribute::
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it is a good idea to keep samples in the tool itself? I was willing to take the sample files out entirely and have a separate download for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's is a good question. I don't think it's done normally that this kind of sample folders are included with the software itself. Separating the sample folder and having a separate download will definitely be a better practice I think. The sample folder size is about ~1.2 MB, so it doesn't make a significant difference in the download size though. Even if you keep the folder inside scancode, having a separate download link will be great (perhaps even with multiple bigger samples) !

Copy link
Member

Choose a reason for hiding this comment

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

I agree with separating out the samples - they are very old and not particularly interesting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, If you could put together better and more interesting samples that'd be great.

..
[ToDo] Research and Write Better

``--max-email`` Options
Copy link
Member

Choose a reason for hiding this comment

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

Should not this be side-by-side with the --email option? (same for the --url option below?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've followed the groupings and order of the options in the help-text as users can easily switch between both without having to search. The present grouping (and in the help-text) is, in my opinion, the most logical one. What can be done is adding Notes to --max-email at -e and the same for the email. I'll do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

This issue has been fixed.

Some important INTEGER values of the ``--license-score INTEGER`` option:

- **0** - Default and Lowest Value, All matches are reported.
- **100** - Highest Value, Only licences with a much better match are reported
Copy link
Member

Choose a reason for hiding this comment

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

We probably need a section on scoring elements for license matches.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I too feel this needs elaboration, and so I've left a ToDo here. But I'd need your/other developers guidance on that. That being said, I still have some unpushed work, and some improvements to make. After that, I was thinking we should mark sections for improvement, and then you/others could guide me/discuss together what can be done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this can be added later and expanded by other developers/users. We can track this with a ticket for now: #28

Copy link
Member Author

@AyanSinhaMahapatra AyanSinhaMahapatra Oct 18, 2019

Choose a reason for hiding this comment

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

@MaJuRG There are some more issues I need help on, along with this one. They all are marked as [ToDo] in the .rst files in their respective locations (Not all [ToDo]s, some of them I need help on). Should I open more tickets for them?

``--license-text`` Options
--------------------------

The ``--license-text`` option includes in the scan result, the matched text for the detected
Copy link
Member

Choose a reason for hiding this comment

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

This is a general comment on the way to craft some sentences:
Here

The --license-text option includes in the scan result, the matched text for the detected
license, under the attribute "matched text".

may be easier to read as:

With the --license-text option, the scan results attribute "matched text" includes the matched text for the detected
license.

@DennisClark what do you think? I am not a native speaker of English!

Copy link
Member Author

Choose a reason for hiding this comment

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

This is really an important issue. I'll look into this and correct stuff all across the docs. Suggestions and help on this is much appreciated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or something like "--license-text: Include the matched licensed text in the scan results for each detected license."

Copy link
Member

Choose a reason for hiding this comment

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

After a join chat today, no major change needed, though it is better to use direct vs. indirect style as general guideline.


A scan example using the ``--license-url-template TEXT`` option ::

./scancode -clpieu --json-pp sample_lic_url_template.json samples --license-url-template https://opensource.org/licenses/{}
Copy link
Member

Choose a reason for hiding this comment

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

That's a great example but the likeliness that a ScanCode TK license key is the same as a the opensource.org key is super low IMHO.
Could you instead use an example using this base URL https://github.com/nexB/scancode-toolkit/tree/develop/src/licensedcode/data/licenses ? and that would point to the .LICENSE file?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. I'll update this.

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is updated.


"reference_url": "https://opensource.org/licenses/zlib",

The referance URL changes for all detected licenses in the scan, across the scan result file.
Copy link
Member

Choose a reason for hiding this comment

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

nit: referance -> reference ? see also this PR title

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing out, I'll resolve all spelling/sentence structure stuff ASAP.

causes the following differance in scan result of the file
``samples/JGroups/licenses/bouncycastle.txt``.

Without Diagnostics::
Copy link
Member

Choose a reason for hiding this comment

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

There is another important thing too is that whole line of text are included (including possibly unmatched parts) by default unless you request license-text-diagnostics

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated this part.

@pombredanne
Copy link
Member

I also see that you added a lot of screen shots. When this is for JSON, it might be better to use plain text instead (and we likely could configure Sphinx to provide some coloring later?) ?

@mjherzog mjherzog changed the title CLI Options Referance for v3.1.1 CLI Options Reference for v3.1.1 Oct 15, 2019
@AyanSinhaMahapatra
Copy link
Member Author

I was using screenshots because in some cases it is easier to hide parts of the text and show the overall structure, but yeah using plain-text and then configuring sounds better! I'll update accordingly.

`Core` Options
==============

.. include:: /scancode-toolkit/rst_snippets/basic_options.rst
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there is a problem with the location of the snippets, per-say. I think @pombredanne just means we should use relative paths if possible? Not sure how sphinx is configured in this case, but changing to ./scancode-toolkit/rst_snippets/basic_options.rst might be enough here?


.. include:: /scancode-toolkit/rst_snippets/core_options.rst

``--generated`` Options
Copy link
Contributor

Choose a reason for hiding this comment

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

@pombredanne it probably should be included in the --info, but this is a scancode-toolkit specific issue I think.

./scancode -clpieu --json-pp sample_generated.json samples --generated

In the results, for each file the following attribute is added with it's corresponding True/False
value ::
Copy link
Contributor

Choose a reason for hiding this comment

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

Some important INTEGER values of the ``--license-score INTEGER`` option:

- **0** - Default and Lowest Value, All matches are reported.
- **100** - Highest Value, Only licences with a much better match are reported
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this can be added later and expanded by other developers/users. We can track this with a ticket for now: #28

``--license-text`` Options
--------------------------

The ``--license-text`` option includes in the scan result, the matched text for the detected
Copy link
Contributor

Choose a reason for hiding this comment

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

Or something like "--license-text: Include the matched licensed text in the scan results for each detected license."


.. image:: data/filter_sample.gif

Componets
Copy link
Contributor

Choose a reason for hiding this comment

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

Spelling error, this should be Components

@AyanSinhaMahapatra
Copy link
Member Author

AyanSinhaMahapatra commented Oct 18, 2019

Pushed some major changes on Options dependency. I'm still working on resolving some issues (Some are solved, I've pointed them out above in individual threads). I'll ping on Gitter channels and re-request reviews when I'm done, possibly today.

Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
@AyanSinhaMahapatra
Copy link
Member Author

@pombredanne @mjherzog @MaJuRG @DennisClark I've pushed fixes on changes requested and resolved all the issues discussed above. Please review.

Signed-off-by: Ayan Sinha Mahapatra <[email protected]>
@AyanSinhaMahapatra
Copy link
Member Author

AyanSinhaMahapatra commented Oct 20, 2019

Another Minor thing, you'd have to go to https://readthedocs.org/dashboard/aboutcode/advanced/, then Default settings -> Requirements file and enter "docs/requirements.txt" for proper ReadTheDocs builds. There is some sphinx version-specific behavior that changes builds, thus this addition. Also, you can head to https://aboutcode-test.readthedocs.io/en/v3.1.1/ to check out the docs.

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.

There is a lot here, as there should be when migrating docs from another source. From a glance, it looks like everything made it over correctly. I would think that this is good enough to merge as-is and more specific corrections and improvements can be handled in individual smaller tickets if needed.

@DennisClark and @pombredanne, thoughts?

@DennisClark
Copy link
Member

@MaJuRG I agree completely with your assessment, thanks.

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.

5 participants