Graphical/usability improvements to cmsMake

[veluca93]

Depends on cms-dev/isolate#4.

[wil93]

👍

[dario2994]

👍

[Harniver]

👍

[gabrfarina]

This is very helpful indeed!

[walypala23]

👍

[Alice94]

👍

[giomasce]

I'm having a look at this. The contribution is mostly good and welcome, but the code quality definitely needs some improvement.

• All our code should be compliant to PEP8 and accepted by pyflakes. Your patch introduces important errors: please, fix them. BTW, do you use our fantastic Git hooks?
• Although I am a big fan of colored output (and CMS logs color scheme was indeed an idea of mine), I definitely cannot stand with magic constants and ANSI commands in the code. Please use the methods starting from https://github.com/cms-dev/cms/blob/master/cms/log.py#L147 to include colors in the terminal output (small patches may be required for what you want to do).

I will add more specific feedback in diff comments.

[veluca93]

It was basically a quick-and-dirty improvement to use during the stage - I
will clean it up as soon as possible.

Most decisions were made considering if something had actually been used
during our experience - for example supporting only GroupMin, aka subtasks,
and (something else), that is assumed to mean a subtask-less task (and thus
mapped in something that has the same score output); this, in fact, covers
all the cases we actually used in our stages.
On Mar 22, 2015 10:28 PM, "Giovanni Mascellani" [email protected]
wrote:

I'm having a look at this. The contribution is mostly good and welcome,
but the code quality definitely needs some improvement.

All our code should be compliant to PEP8 and accepted by pyflakes.
Your patch introduces important errors: please, fix them. BTW, do you use
our fantastic Git hooks
https://github.com/cms-dev/cms/wiki/Developerhints#git-hooks?

Although I am a big fan of colored output (and CMS logs color scheme
was indeed an idea of mine), I definitely cannot stand with magic constants
and ANSI commands in the code. Please use the methods starting from
https://github.com/cms-dev/cms/blob/master/cms/log.py#L147 to include
colors in the terminal output (small patches may be required for what you
want to do).

I will add more specific feedback in diff comments.

—
Reply to this email directly or view it on GitHub
#414 (comment).

[giomasce]

It definitely seemed to be quick-and-dirty. Let me just notice that there is not much point to creating a PR and adding half a dozen +1 to it if you are aware that it still requires work...

However, other than the hundreds of details that I pinpointed, it appears to be ok (you may also want to squash the commits together, since there is more or less nothing that separates them).

[veluca93]

I agree on that - I had some peer pressure :P
On Mar 22, 2015 11:20 PM, "Giovanni Mascellani" [email protected]
wrote:

It definitely seemed to be quick-and-dirty. Let me just notice that there
is not much point to creating a PR and adding half a dozen +1 to it if you
are aware that it still requires work...

However, other than the hundreds of details that I pinpointed, it appears
to be ok (you may also want to squash the commits together, since there is
more or less nothing that separates them).

—
Reply to this email directly or view it on GitHub
#414 (comment).

[wil93]

Let me just notice that there is not much point to creating a PR and adding half a dozen +1 to it if you are aware that it still requires work

That was the buzz of having a really nicer cmsMake that we all locally patched immediately, and the +1 wave was a display of that (just some CMS love 😄)

On a side note: I was under the impression that cmsMake was almost a separated piece of CMS and didn't really require its code standards, I see that I was wrong (BTW: it would be awesome to brainstorm a polygon-like cmsMake, or convince polygon authors to release their code 😛)

[stefano-maggiolo]

Well, I admit that the code quality is much lower, but that's not a good reason to include more code with low code quality :)

[giomasce]

Definitely +1 for Stefano.

[giomasce]

Merged, at last. Backers, please be happy! :-)