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

node_classes.py has too many lines #465

Closed
AWhetter opened this issue Nov 5, 2017 · 5 comments · Fixed by #1223
Closed

node_classes.py has too many lines #465

AWhetter opened this issue Nov 5, 2017 · 5 comments · Fixed by #1223

Comments

@AWhetter
Copy link
Contributor

AWhetter commented Nov 5, 2017

I am about to merge #458 but it is failing the pylint check because the docstrings have made the module too big. I didn't want to start moving things around so for now I have disabled the message, until we figure out how we want things laid out.

@PCManticore
Copy link
Contributor

We could definitely start splitting it up, but I don't mind having the error disabled for now.

@brycepg
Copy link
Contributor

brycepg commented Mar 28, 2018

What do you think about making nodes a subpackage, moving nodes.py into nodes/__init__.py
Move node_classes and scope_nodes into this subpackage. For important nodes like NodeNG we could create single files per class e.g. nodes/nodeng.py , and move some of the assist functions into a nodes/util.py.
This would help with organization, split up large files, and prepare for #329

@PCManticore
Copy link
Contributor

I wouldn't mind some of these refactorings, @brycepg although I'm not sure we'll ever do #329

@PCManticore
Copy link
Contributor

I think this is fine as it is right now, as per my reasoning from #587. If the file would grow larger than it is right now, I'd say we should split it, but not the nodes themselves, rather any functionality that doesn't necessarily belong there.

@Pierre-Sassoulas
Copy link
Member

Reopening because of the argument here : #587 (comment)

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 a pull request may close this issue.

4 participants