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

An app to detect licenses from the provided input license text #450

Draft
wants to merge 79 commits into
base: main
Choose a base branch
from

Conversation

lf32
Copy link

@lf32 lf32 commented Jun 16, 2022

The aim of this PR is to create a working app inside this Django project to detect licenses from the provided input text and summarize the results for the end user.

@lf32 lf32 requested a review from tdruez June 16, 2022 05:27
Copy link
Contributor

@tdruez tdruez left a comment

Choose a reason for hiding this comment

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

@lf32 That's a good start, see my comments for minor improvements.

In the future you need to provide more context than simply requesting a review.
There's a tons of commented code, which part do you need to be reviewed?
Include what you have accomplished and what is left to do, also include the issues you are facing.

scancodeio/settings.py Outdated Show resolved Hide resolved
scancodeio/urls.py Outdated Show resolved Hide resolved
scantext/admin.py Outdated Show resolved Hide resolved
scantext/apps.py Outdated Show resolved Hide resolved
scantext/templates/scantext/license_detail.html Outdated Show resolved Hide resolved
@lf32
Copy link
Author

lf32 commented Jun 16, 2022

@lf32 That's a good start, see my comments for minor improvements.

In the future you need to provide more context than simply requesting a review. There's a tons of commented code, which part do you need to be reviewed? Include what you have accomplished and what is left to do, also include the issues you are facing.

thanks, i pinged you to let you know about the PR

  • the scancode.api.get_licenses accepts files as input but not text, what do you think of it?
  • do you think the input text should be stored as a file and then pass it as the argument?

@tdruez
Copy link
Contributor

tdruez commented Jun 16, 2022

You can use a tempfile for now or adapt the code to use match with the query_string argument?

Still checking this! will let you know ASAP

@tdruez
Copy link
Contributor

tdruez commented Jun 16, 2022

Screenshot 2022-06-16 at 12 15 58

You are putting side by side a form submit button "Scan License" and a navigation link "New Project".

I think the "New Project" as no need to be present in that view.
Also, the action button "Scan License" would make more sense to be located after the form, not on top of it.
The textarea is simply too big, reduce the height.

@lf32
Copy link
Author

lf32 commented Jun 19, 2022

@tdruez please suggest changes and also any resource for recommending license?

res_license

@lf32
Copy link
Author

lf32 commented Jun 19, 2022

Screenshot 2022-06-16 at 12 15 58

You are putting side by side a form submit button "Scan License" and a navigation link "New Project".

I think the "New Project" as no need to be present in that view. Also, the action button "Scan License" would make more sense to be located after the form, not on top of it. The textarea is simply too big, reduce the height.

res_license

How is this?
rows are set to 25, isn't it ok?

@tdruez
Copy link
Contributor

tdruez commented Jun 20, 2022

please suggest changes

This looks awesome but where is the code? No code, no review possible.

any resource for recommending license?

I don't know what you mean by that.

* Dropdown cards work well in the details page
* Re-arranged html code
* Changed Scan path from /scan/ to /scantext/

Signed-off-by: Akhil Raj <[email protected]>
@lf32
Copy link
Author

lf32 commented Jun 20, 2022

This looks awesome but where is the code? No code, no review possible.

I have just pushed changes over here

Copy link
Contributor

@tdruez tdruez left a comment

Choose a reason for hiding this comment

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

@lf32 that's a great start, see my comments for minor improvements.

I think the UI is fine for now, you should focus on the views code to make it working on real input text.

scanpipe/templates/scanpipe/base.html Outdated Show resolved Hide resolved
scantext/forms.py Outdated Show resolved Hide resolved
scantext/forms.py Outdated Show resolved Hide resolved
scantext/templates/scantext/license_detail.html Outdated Show resolved Hide resolved
scantext/templates/scantext/license_scan.html Outdated Show resolved Hide resolved
scantext/templates/scantext/license_detail.html Outdated Show resolved Hide resolved
scantext/templates/scantext/license_scan.html Outdated Show resolved Hide resolved
@lf32 lf32 force-pushed the lf32-licensetext-devel branch from 564b5fe to 49fe7a5 Compare June 20, 2022 07:05
@tdruez
Copy link
Contributor

tdruez commented Jun 20, 2022

@lf32 please read the following for future commit messages (no need to update the existing ones): https://aboutcode.readthedocs.io/en/latest/contributing/writing_good_commit_messages.html

For example: "changed card title from name to short_name" should be "Change card title from name to short_name #450"

You can look at the commit history of the repo and try to follow the conventions in place.

scantext/views.py Outdated Show resolved Hide resolved
lf32 added a commit that referenced this pull request Jun 20, 2022
* Set Input Textarea to 15
* Added file-upload into form
* Changed <pre> to <textarea> in the details page
* Modified Navigation Bar and Renamed Scan Again Button

Signed-off-by: Akhil Raj <[email protected]>
lf32 added a commit that referenced this pull request Jun 20, 2022
* license_scanview function uses tempfile to run license detection on the
  provided input license text

Signed-off-by: Akhil Raj <[email protected]>
lf32 added a commit that referenced this pull request Jun 20, 2022
* license_scanview function uses tempfile to run license detection on the
  provided input license text

Signed-off-by: Akhil Raj <[email protected]>
@lf32 lf32 force-pushed the lf32-licensetext-devel branch from 7e11c91 to d3dbb17 Compare June 20, 2022 18:45
lf32 added a commit that referenced this pull request Jun 20, 2022
* license_scanview uses tempfile to run license detection on the
  provided input license text

Signed-off-by: Akhil Raj <[email protected]>
@lf32 lf32 force-pushed the lf32-licensetext-devel branch from d3dbb17 to 64ffe39 Compare June 20, 2022 18:47
* Set Input Textarea to 15
* Added file-upload into form
* Changed <pre> to <textarea> in the details page
* Modified Navigation Bar and Renamed Scan Again Button

Signed-off-by: Akhil Raj <[email protected]>
@lf32 lf32 force-pushed the lf32-licensetext-devel branch from 64ffe39 to cb9de2d Compare June 20, 2022 18:54
* license_scanview function uses tempfile to run license detection on the
  provided input license text

Signed-off-by: Akhil Raj <[email protected]>
@tdruez
Copy link
Contributor

tdruez commented Oct 14, 2022

Hi @lf32 what's the latest status on this PR?

@lf32
Copy link
Author

lf32 commented Oct 14, 2022

@tdruez I am writing the tests.

@lf32
Copy link
Author

lf32 commented Oct 14, 2022

  • write unit tests
  • set a fixed width for the dropdown
  • add navigation with prev and next licenses

@tdruez
Copy link
Contributor

tdruez commented Oct 14, 2022

@lf32 Thanks for the update!

@pombredanne pombredanne added this to the v33.0.0 milestone Dec 8, 2022
@pombredanne pombredanne mentioned this pull request Jan 12, 2023
1 task
@DennisClark DennisClark self-requested a review January 12, 2023 16:57
@DennisClark DennisClark self-assigned this Jan 12, 2023
@DennisClark
Copy link
Member

per @pombredanne we need to determine if this properly belongs in ScanCode.io or in a separate application. The design-to-date is a separate app.

lf32 added 3 commits January 25, 2023 14:00
This feature helps in working with files of type
text or other (example: binary)

Signed-off-by: Akhil Raj <[email protected]>
@lf32
Copy link
Author

lf32 commented Feb 5, 2023

@AyanSinhaMahapatra, I have added a utilities dropdown and moved scan tab to Detect License which redirects to /scantext/.
utility

@tdruez
Copy link
Contributor

tdruez commented Feb 17, 2023

Hi @lf32 what's the latest status on this PR? Is it feature complete?

@lf32
Copy link
Author

lf32 commented Feb 17, 2023

@tdruez I have discussed with philippe about it and still needs changes to be made.

@tdruez
Copy link
Contributor

tdruez commented Feb 17, 2023

@lf32 Can you provide a list of those changes?

@lf32
Copy link
Author

lf32 commented Feb 17, 2023

  • writing more tests
  • code correction
  • migrating to scancode-toolkit v32.0.1 in scantext
  • more efficient user interface

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.

4 participants