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

added new function-extractAssignedSring #42

Merged
merged 1 commit into from
Oct 17, 2021

Conversation

Yoda-Canada
Copy link
Contributor

added a new function named "extractAssignedSring" in binder.py file.

nirjas/binder.py Outdated Show resolved Hide resolved
nirjas/binder.py Outdated
Return the content of the string.
'''
content = []
regex = r'(?<=(=\s*\"))(.*?)(?=(\"))'
Copy link
Member

Choose a reason for hiding this comment

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

In most scripting languages like PHP and JS, a string can also be denoted with single quotes $var = 'valid PHP string' ;.
Would it make sense to capture both ['"] and while creating output, ignore anything of length less than 2 (like C characters)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the code. I wonder if it meets the requirements.

Copy link
Member

Choose a reason for hiding this comment

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

It should work. Just a suggestion to check output length before appending it to content to eliminate stuff like char c='c';. Something like following

if output and len(output) > 1:
    content.append([line_number, output.strip()])

https://github.com/fossology/Nirjas/pull/42/files#diff-f2e6844099e55c848909950ff4d101df81ebad6965f701033824ddc1960c7ca7R173-R174

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the output. Could you merge this PR? Thx.

Copy link
Member

Choose a reason for hiding this comment

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

Hey @Yoda-Canada , There are few extra empty lines here and there, Also updating the documentation with the changes might be a great help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kaushl2208 I have deleted the extras empty lines. Does the documentation mean README.md - documentation part? If it is, could you create a new issue for documentation? I need time to study this part because some flags haven't been introduced yet.

Copy link
Member

@Kaushl2208 Kaushl2208 left a comment

Choose a reason for hiding this comment

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

Hey, Can You also update the documentation regarding the update you have made?

nirjas/binder.py Show resolved Hide resolved
@Yoda-Canada
Copy link
Contributor Author

@Kaushl2208 I cannot edit the "https://github.com/fossology/Nirjas/wiki" page, and it also cannot clone to my repo. I only can edit the README.md, but cannot edit the linked page in the REDAME.md. Could you merge this PR and create a new issue for documentation? Thank you.

@Kaushl2208
Copy link
Member

Also, You can access the Readme and edit it. For wiki I will just add the new issue?

@Kaushl2208
Copy link
Member

Hey @Yoda-Canada , It will be great if you can squash all commits into one. We try to keep change history clean and easy to maintain.

Rest everything looks good to me :D

@Yoda-Canada
Copy link
Contributor Author

@Kaushl2208 I have squashed all commits to one. Please test it ASAP. Thx.

@Kaushl2208
Copy link
Member

@Kaushl2208 I have squashed all commits to one. Please test it ASAP. Thx.

Hey @Yoda-Canada , I will test it and merge it! Thank you for your contributions.

@GMishx GMishx merged commit 92a89be into fossology:master Oct 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants