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

style, structure, and tests #1

Open
chadrik opened this issue Aug 27, 2014 · 4 comments
Open

style, structure, and tests #1

chadrik opened this issue Aug 27, 2014 · 4 comments

Comments

@chadrik
Copy link

chadrik commented Aug 27, 2014

Hi folks,
First of all, thanks for the really cool package.
Before people start forking this project it would be good to address a few issues which will end up causing merge conflicts if you try to deal with them later.

first, you should probably convert tabs to space-tabs (this is part of the python PEP 8 style guide, and is at this point very standard for python code). one of your 2 forks has already done precisely this, which means their code won't merge with your other forks. that's why it's important to do this now.

second, it would be good restructure the code into a python package and get some standard root folders setup. something like this:

dependsworkflow/
  bin/  <-- executables go here
  depends/   <-- this is the root of your python package
    __init__.py  <-- typically no code goes in here
    ui/
      __init__.py  <-- typically no code goes in here
      scene_graph_widget.py
      main_window.py
      etc...
    core/
      __init__.py  <-- typically no code goes in here
      dag.py  <-- remove the depends_ prefix
      node.py
      data_package.py
      etc...
    utils/
      __init__.py  <-- typically no code goes in here
      etc...
  tests/
  examples/

being completely honest, it's a little scary seeing a project with so much code that is not organized into a python package, since this is a pretty fundamental concept. getting your code organized with instill confidence and gain contributors. again, making these structural changes will mean other peoples forks won't merge cleanly, so it's best to take care of soon.

tests would also be great to add, particularly for the core (testing qt ui is possible, but not as important). you have some options for creating and running tests. the unittest module comes in the python standard library. there's a backport of the python 3 version called unittest2 that has a few more features. also, there are tools to help ease the discovery of tests, which include nose and tox.

thanks, and keep up the good work.

@andrew-gardner
Copy link
Owner

Good stuff, Chad, thanks for the message!

I've sent a pull request to the developer who fixed the tabs. Hopefully that will come along soon.

The package structure makes a lot of sense - thanks for pointing towards something sane. Chalk the current structure up to inexperience on my part. I'll look into the restructure, alert those who have forked, and make something a lot more standard than what's currently there. With some luck, I'll find time to have that done before the end of the week.

/Andrew

@andrew-gardner
Copy link
Owner

Before I hit the road for a couple of weeks, I've made some baby steps towards a better directory structure.

The beginnings of a Python package (PyPI compatible) have been provided by user mottosso, but a larger module rearrange is to follow. Unit tests and examples will accompany the rearrange.

Thanks for the proposed module hierarchy too, Chad. It makes sense, but will take more time than the 30 minutes I currently have. It should go quickly once I have the spare cycles to dedicate to it.

And finally, thanks again for the comments and the interest. This continues to be a great learning experience for all of us here and we plan on incorporating as much feedback as we can.

@nerdvegas
Copy link

Hey Andrew,

I've just come across your project. My work has a very similar internal project but I'd like to know more about yours - it may make sense to move forward using an open source project, if it supports the same feature set that we need.

Chad beat me to it and has already asked many of the questions / given the same suggestions I was going to give. But here are my own thoughts anyway, I hope they are useful.

  • +1 for splitting the UI from the core API into its own directory. My first concern looking at the project was not knowing what I can and cannot do without involving UI code. In order for this project to be useful I have to know that I can use it as an API, as well as a tool.
  • +1 for unit tests. They will same tremendous amounts of pain in the long run for all contributors involved, and will keep your project bug-free more often.
  • Related to changing the project structure - once that's done, all your imports should be absolute, and should only import the symbols you need eg:
from depends.variables import foo, bah
# rather than
# import depends_variables
  • This is off-topic for this thread, but - inputs/outputs can only be 'data packets', which are basically file references? And only 'attributes' can be POD types (int strings etc)? Isn't this too much of a limitation - why not model attributes as inputs, and have data packets able to be any data type, rather than just file references? Not being able to use the output of one node as an attribute on another will be a significant limitation for us I think. Merging attributes and inputs/outputs would turn this into a more general processing engine, which would be great! But perhaps that is not the goal of this project? I would say that in general, process graphs tend to be modelled as stateless nodes with inputs/outputs, where attributes are not treated as a different class of data.

Thanks,
Allan

@andrew-gardner
Copy link
Owner

Hey Allan!

First, apologies for the super-late reply. I'm on vacation now and this is the first time I've had fast access to the interwebz in ~15 days.

Second, your and Chad's suggestions are all great. As you've probably noticed, I never had a chance to really sit down and refactor things as Chad described. Actually, another group had an depends-like program internal to their company and pulled out the Depends UI and stuck that on top of their core - so, just as you say, doing this separation is really useful, and it didn't sound particularly hard, so maybe I did something right.

Unit tests would be great as well. When I was developing things, I had a bunch of our own workflows, data packets, and node types that I would hand-unit-test with, but that's not practical for an open source project, so a big yes to those.

Also, yeah, that "depends_" stuff in the imports really needs to go away. It was the next step in my refactor that got prioritized so low (relative to other projects at work) that it just never happened.

Hopefully my delay in responding didn't turn you off too much. We use Depends as it is, and if you think it has features that would be useful to you, and you want to add more, we would love your contributions to the project! This includes what you describe with data packets. All we've needed so far were filenames, but yes, they could (and should) be any POD type. That's a pretty major step towards making Depends something more than a tool that a small group of researchers use to organize our workflows. Our needs are minimal, and keeping our limited use cases working should be quite easy if you add more interesting functionality. So, please, feel free to add to Depends if you think the code that is already there will save you some time.

I'll be returning home by Monday if you would like to chat about it a bit more. Until then, I hope the weather's turning nice wherever you are, Allan, and I'll type with you more soon.

/Andrew

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

No branches or pull requests

3 participants