Skip to content

Conversation

rpotter12
Copy link

  • add code to handle podspec file.
  • update scripts to detect all types of dependencies from the file.

Copy link

@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.

@rpotter12 Are the various formatting changes done by you or by some formatter tool?

Also, we probably want to Add or Update the README file to explain the reason for the fork and what exactly is different than gemfileparser

@rpotter12
Copy link
Author

@rpotter12 Are the various formatting changes done by you or by some formatter tool?

@MaJuRG All the formatting changes are done by me only. :)

Also, we probably want to Add or Update the README file to explain the reason for the fork and what exactly is different than gemfileparser

Okay. I will update the README file soon :)

@rpotter12 rpotter12 requested a review from steven-esser July 16, 2020 05:12
Copy link

@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.

@rpotter12 Since we fork, I am not sure we want to have stuff like syntax changes. I think in general, formatting changes like this are a good thing. I am more concerned about having a clean diff in case we want to merge this in upstream in the future.

@pombredanne What is your take here?

@rpotter12 Other than that particular minor issue, this looks good to me.

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 only have a few nits for your review... but most and foremost we need to have tests for these before we can merge.


def __init__(self):
self.name = None
self.name = ''
Copy link
Member

Choose a reason for hiding this comment

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

Why use empty strings instead of None?

Copy link
Author

Choose a reason for hiding this comment

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

I think the repo which we have forked is some commits behind. Using empty strings here are the original changes in the upstream repo. And may be this is helpful in extracting data from gemfile files.

self.group = None
self.platform = None
self.platforms = []
self.groups = []
Copy link
Member

Choose a reason for hiding this comment

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

I guess you removed these attributes because they are either not in the spec or seldom used or parsable?
platform is still a thing though rarely there alright https://guides.rubygems.org/specification-reference/#platform=
Can you confirm?

I never saw a group in a spec so far... did you?

Copy link
Author

@rpotter12 rpotter12 Jul 21, 2020

Choose a reason for hiding this comment

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

I think they are used in gemfile files. gemfileparser is for both gemspec and gemfile but I think @balankarc have implement these in another way so he removed this in the original repository :)

def parse_line(self, line):
'''Parses each line and creates dependency objects accordingly'''
"""
Parses each line and creates dependency objects accordingly.
Copy link
Member

Choose a reason for hiding this comment

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

Please use the imperative style for docstrings.

Copy link
Author

Choose a reason for hiding this comment

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

I will change it soon :)

dep.parent = []
else:
linefile = io.StringIO(line) # csv requires a file object
for line in csv.reader(linefile, delimiter=','):
Copy link
Member

Choose a reason for hiding this comment

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

Can you remind my the benefits of using a CSV module here?

Copy link
Author

Choose a reason for hiding this comment

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

I think @balasankarc used this CSV module here because dependency contains name and different version numbers and they are seperated by comma and they have to split by comma and if we use something else then to remove extra character the code will become more complex. CSV module might have make this easy to remove extra characters. I guess this might be the only good solution to use CSV module here.

'download_url': 'https://gitlab.com/balasankarc/gemfileparser',
'author_email': 'balasankarc@autistici.org',
'description': "A library to parse Rubygem gemspec and Gemfile files and Cocoapods podspec and Podfile files using Python. Friendly fork of https://gitlab.com/balasankarc/gemfileparser",
'author': 'nexB',
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the original author name there. Or something like `Rohit Potter for nexB based on original work of Balasankar C' ... but in all cases we want Balasankar to get proper credits

Copy link
Author

Choose a reason for hiding this comment

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

Okh :)
I will change this in new commits.

Choose a reason for hiding this comment

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

In case we get to collaborate and move development upstream as suggested in #2 (comment), this can definitely change to an AUTHOR file based logic so that every contributor gets credit. 🙂

@pombredanne
Copy link
Member

Never mind the auto close.... that's Github playing tricks on us.

@balasankarc
Copy link

@pombredanne @rpotter12 Wanted to open an issue about this, but since issue tracker is not enabled here I am "hijacking" this thread. 😀 Sorry about that.

If you are interested, I am more than happy to move gemfileparser into an organization and add you to it, so instead of maintaining a fork you can use the upstream repository itself. I am not sure about how much time I might be able to dedicate in maintaining it though, but if the project is of use to you, I think it is only right that you get to be an upstream contributors/co-authors.

@rpotter12
Copy link
Author

rpotter12 commented Jul 24, 2020

@balasankarc Thank you for your reply :)
gemfileparser is very helpful in the scancode-toolkit software. I think you can transfer ownership to @pombredanne or make us maintainer of that repo if you are not able to dedicate much time on it. We also need to update the Pypi version also because it is having the very old version gemfileparser and needs to be updated regularly.

Thank you so much for you reply :)

@pombredanne
Copy link
Member

@balasankarc re

if you are interested, I am more than happy to move gemfileparser into an organization and add you to it, so instead of maintaining a fork you can use the upstream repository itself. I am not sure about how much time I might be able to dedicate in maintaining it though, but if the project is of use to you, I think it is only right that you get to be an upstream contributors/co-authors.

That would be great!

We can either create a new org or we can host it under nexB org as this is meant for use directly in https://github.com/nexB/scancode-toolkit which is in the same org. (ScanCode is a tool that you should consider for use in Gitlab ... even if this is Python and not Ruby as this is the best tool out there for license detection ;)

Note that I would like also to fold in that gemfileparser library the support we have for Gemfile.lock in here https://github.com/nexB/scancode-toolkit/blob/develop/src/packagedcode/gemfile_lock.py

So your call in anycase.
AFAIK the latest code is in your gitlab repo and not in Github ;).

@balasankarc
Copy link

  1. Moved upstream to https://github.com/gemfileparser/gemfileparser
  2. Synced GitHub with GitLab. Now both of them should have same code.
  3. Added @pombredanne as an owner to https://github.com/gemfileparser so the needful can be done without depending on me.

@pombredanne If you have a PyPi profile, please share your email address and I can add you as a Maintainer to the package there too so you can update the package.

I will update README and add an AUTHOR file and everything in upstream repo soon.

@pombredanne
Copy link
Member

pombredanne commented Jul 25, 2020 via email

@pombredanne pombredanne reopened this Jul 25, 2020
@pombredanne
Copy link
Member

Closing these as we eventually used and pushed all these upstream!

@pombredanne pombredanne closed this Sep 8, 2022
pombredanne pushed a commit that referenced this pull request Sep 8, 2022
* Add PEP 517/518 pyproject.toml file
* Add setuptools_scm to handle versioning
* Add setup.py content to setup.cfg
* Update setup.py to act as a shim (so pip install -e works)

Addresses: #2

Signed-off-by: Steven Esser <sesser@nexb.com>
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