-
Notifications
You must be signed in to change notification settings - Fork 174
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
Updated readme and added notebook #845
Conversation
Signed-off-by: Yash kalathiya <[email protected]>
A few comments to @yash-kalathiya: The notebook and README are generally fine, but my comments are about consistency with the other transform README and notebooks:
|
As per you said, I have changed readme file and also changed notebook directory. |
@yash-kalathiya Thanks for making all the directory changes, but now I get errors running the notebook, as it doesn't find the transform. I have Python 3.10 on my laptop (I have 3.12 too, but not 3.11). Requirement already satisfied: six in ./venv/lib/python3.10/site-packages (from extractcode>=31.0.0->extractcode[full]>=31.0.0->scancode-toolkit==32.1.0->dpk_header_cleanser_transform_ray==0.2.3.dev0) (1.17.0) |
@yash-kalathiya I think the problem for my environment is still the extractcode package that gives the error below: Using cached dockerfile_parse-2.0.1-py2.py3-none-any.whl.metadata (3.3 kB) Can you please try: |
@yash-kalathiya I pushed a "fixed" version of the code to the branch that successfully ran on my local environment. The 3 changes that I made are:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve the functionality, after the latest changes I made.
Thank you, @shahrokhDaijavad , for resolving the version issue. I sincerely apologize for not being able to assist earlier as I was occupied with other work. I truly appreciate your efforts and understanding. |
|
@Param-S Do you have time to review this? If you are ok with the review and approve, are you authorized to merge? If not, I will ask Maroun. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@yash-kalathiya can you resolve the conflict in readme file & push the changes. Thanks |
Signed-off-by: Yash kalathiya <[email protected]>
c99d964
to
9bdc7dd
Compare
Signed-off-by: SHAHROKH DAIJAVAD <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yash-kalathiya Somehow, you went back to the version of Notebook that didn't have pip install scancode-toolkit in it and as I had said previously, without this, it won't work in my venv. So, I added this again and also fixed some broken links in the README file.
@Param-S I recommend merging this. I am not authorized to merge. |
Why are these changes needed?
To add Google colab compatible notebook and enhance readme file.
Related issue number (if any).
844