Skip to content

Conversation

@boegel
Copy link
Member

@boegel boegel commented Jan 21, 2014

rework function/method signatures in main.py and parallelbuild.py to follow build_options/build_specs scheme used for EasyBlock/EasyConfig class constructors

…follow build_options/build_specs scheme used for EasyBlock/EasyConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this set again? force = True is set above already?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, you're right, I somehow missed that... will clean this up

@stdweird
Copy link
Contributor

@boegel some minor remarks, but excellent cleanup. now all that's left is to move some functions out of main in new module!

@boegel
Copy link
Member Author

boegel commented Jan 22, 2014

@stdweird: I'll take the opportunity here to move some stuff around. I already moved cleanup_logfile to an inner function in the main function (doesn't make much sense to move that to an external module I feel).

boegel added 21 commits January 22, 2014 10:41
….tools, move process_easyconfig from main to easyconfig.tools
… regtest from main to parallelbuild module, get_easyblock_instance from parallelbuild to easyblock module
@boegel
Copy link
Member Author

boegel commented Jan 22, 2014

@stdweird: I finished the moving around of functions out of main.py, which now only features the main function, nothing else.
Unit tests are happy, and so is PyLint (mostly). I was very careful when moving stuff around, so I'm fairly confident I didn't break too much...

@stdweird
Copy link
Contributor

@boegel it's an improvement for main, that's true, but easyblock is now 2k+ lines!!!! that's absurd. modules over 1k lines are not readable by any standard imho.

@boegel
Copy link
Member Author

boegel commented Jan 22, 2014

@stweird: By moving stuff out of main.py, only about 300 lines were added to easyblock.py.
There currently are only 6 functions in easyblock.py, and one class definition for EasyBlock. The latter already consumes about 1800 lines by itself...

I understand that you feel that modules should be kept of reasonable size, but at an extreme you can put every function/class in a separate module. I don't think that helps a lot in keeping things organized well...

Here are the current largest modules:

$ find easybuild -name '*.py' | xargs wc -l | sort -s | tail -10
     619 easybuild/tools/config.py
     638 easybuild/tools/options.py
     695 easybuild/framework/easyconfig/easyconfig.py
     792 easybuild/tools/modules.py
     797 easybuild/framework/easyconfig/format/version.py
    1090 easybuild/framework/easyconfig/tools.py
    1373 easybuild/tools/filetools.py
    2250 easybuild/framework/easyblock.py
    2471 easybuild/tools/configobj.py
   23614 total

It doesn't look that bad, imho, especially since 80% of easyblock.py is the 'abstract' EasyBlock class.

For now, I'd leave it at this. This took quite a bit of effort (several hours over the last couple of days) to get to this point, avoiding circular dependencies between modules and trying to make sure nothing was being broken in the process...

I suggest to try and cook up some well defined style guidelines on size of functions, total size of modules and maybe also number of functions per module, before we start breaking up easyblock.py in random multiple modules (remember the graph of Python modules in EasyBuild?).

…hat returns None on parsing failure, and fix minor issues reported by PyLint
@stdweird
Copy link
Contributor

now tracked in #816. i also opened #817 since i saw the buildstats function pass by.
@boegel ready to merge imho

@boegel
Copy link
Member Author

boegel commented Jan 23, 2014

fix a couple of minor issues, but nothing really major, so merging this all in, thanks for the review @stdweird

boegel added a commit that referenced this pull request Jan 23, 2014
rework function/method signatures in main.py and parallelbuild.py
@boegel boegel merged commit f823c78 into easybuilders:develop Jan 23, 2014
@boegel boegel deleted the refactor_function_signatures_main branch January 23, 2014 21:21
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.

2 participants