Skip to content

Conversation

@alexbarrera
Copy link
Member

This dev branch addresses a couple things needed moving forward:

  • Migrate code to Python 3 (worth noting that the new code will not be compatible with Python 2.x anymore)
  • Add setup.py and some other necessary file for packaging the tool and make it installable with pip install /path/to/ggr-cwl-ipynb-gen

Since we don't have unittests or CI yet, it might be one for one of the reviewers to download and test before merging (if you haven't yet)

Copy link
Member

@tncowart tncowart left a comment

Choose a reason for hiding this comment

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

In addition to my comments I don't think you should check in the symlinks.

@@ -0,0 +1,2 @@
if __name__ == '__main__':
ggr-main()
Copy link
Member

Choose a reason for hiding this comment

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

ggr-main() isn't a valid identifier. I think you can just delete this.

setup.cfg Outdated
@@ -0,0 +1,18 @@
[metadata]
name = ggr-cwl-ipynb-gen-alexbarrera
Copy link
Member

Choose a reason for hiding this comment

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

should you remove -alexbarrera?

setup.cfg Outdated
description = IPython notebook generator for GGR CWL processing pipelines of genomic data
long_description = file: README.md
long_description_content_type = text/markdown
url = https://github.com/alexbarrera/ggr-cwl-ipynb-gen
Copy link
Member

Choose a reason for hiding this comment

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

the urls should be updated.

setup.py Outdated
url='https://github.com/alexbarrera/ggr-cwl-ipynb-gen',
author='Alejandro Barrera',
author_email='alejandro.barrera@duke.edu',
classifiers=[
Copy link
Member

Choose a reason for hiding this comment

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

This list doesn't match the list in setup.cfg

setup.py Outdated
('templates', ['ggr_cwl_ipynb_gen/templates/ungzip_fastq_files.j2']),
], # Optional
project_urls={
'Bug Reports': 'https://github.com/alexbarrera/ggr-cwl-ipynb-gen/issues',
Copy link
Member

Choose a reason for hiding this comment

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

these urls should be updated to the new location in reddylab.

requirements.txt Outdated
ruamel.yaml >=0.11.11

setuptools
pymongo No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't pymongo be versioned too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't versioned pymongo because I'm not sure where to draw the line of the oldest version that should be accepted

Copy link
Member

Choose a reason for hiding this comment

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

Your initial commit was 4/26/2017 and it looks like the newest version of pymongo at the time was 3.4.0, so probably that version (it's only at 3.11 now, 4 years later).

requirements.txt Outdated
xlrd >=1.0.0
ruamel.yaml >=0.11.11

setuptools
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need setup tools here because it's mentioned in pyproject.toml

MANIFEST.in Outdated
@@ -0,0 +1 @@
include templates/* No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need a manifest file because you use data_files in setup.py

@alexbarrera
Copy link
Member Author

What's the argument to remove the symlinks? I wanted to keep them for backwards compatibility, although admittedly this should be work with Python 2.x, so perhaps I shouldn't worry... Curious about the justification to remove them though

@tncowart
Copy link
Member

Just to declutter the repo. I don't feel super strongly about it though.

@alexbarrera
Copy link
Member Author

alexbarrera commented Mar 15, 2021

Got it, yeah, I imagined that was it. I also feel like symlinks are prob something that can stay away from the repo, not common to find them committed. I just removed them.

Thanks for all the comments!!

@alexbarrera alexbarrera merged commit 84314cc into master Mar 15, 2021
@tncowart tncowart deleted the dev branch March 15, 2021 22:20
@tncowart
Copy link
Member

Closes #11

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.

3 participants