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
Add ability to generate .spec files from local PKG-INFO file #189
Conversation
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.
Looks mostly okay. I've just left some suggestions
data['source_url'] = (args.source_url or | ||
(durl and durl['url']) or | ||
args.name + '-' + args.version + '.zip') | ||
source_url = data['source_url'] = (args.source_url or (durl and durl['url'])) |
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.
This change is just a movement of code and adds the lines 382 and 383. I think it's not needed, just keep it as it is.
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.
Sometimes we place our generated source files archive under different file paths, provided by --source-glob parameter. it would be nice to take this into account in our code, and set source_url as the basename of our generated source files archive
source_glob = '%{name}-%{version}.*' | ||
data_name = data['name'] or name | ||
|
||
tarball_file = [] | ||
for __name in (name, name.translate(tr), data_name, data_name.translate(tr)): | ||
tarball_file.extend(glob.glob(replace_string(source_glob, {'name': __name, 'version': version}))) | ||
if tarball_file: | ||
break |
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.
Why do you use this new function replace_string
instead of just keeping the python format for this simple replacement?
source_glob = '%{name}-%{version}.*' | |
data_name = data['name'] or name | |
tarball_file = [] | |
for __name in (name, name.translate(tr), data_name, data_name.translate(tr)): | |
tarball_file.extend(glob.glob(replace_string(source_glob, {'name': __name, 'version': version}))) | |
if tarball_file: | |
break | |
source_glob = '{name}-{version}.*' | |
data_name = data['name'] or name | |
tarball_file = [] | |
for n in (name, name.translate(tr), data_name, data_name.translate(tr)): | |
tarball_file.extend(glob.glob(source_glob.format(name=n, version=version))) | |
if tarball_file: | |
break |
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.
Because Python's 'format' method for replacement explicitly replaces all elements with {name}, but sometimes we need to leave {name} unchanged, also Python's 'format' method does look for neither % symbol, nor %% symbol.
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 don't get why it's required to be able to look for {name}
or something like that in the glob search for filenames, but if that is your user case, it's okay, not a big deal.
Co-authored-by: danigm <daniel.garcia@suse.com>
Co-authored-by: danigm <daniel.garcia@suse.com>
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
I've done the merge manually to squash all the commits in one. 2af7c24 |
No description provided.