Update project internals #36

mimischi · 2018-02-13T13:32:11Z

Changes made in this pull request:

Added Makefile to project.
Updated help text and docstrings for generate, submit and analyze.
Rephrased and formatted messages printed to the console.
mdbenchmark generate will now first look for the .tpr file and only then do everything else.
Added unit tests for all messages generated via click.echo() and click.BadParameter().
Add unit test for guess_ncores() utility function.

PR Checklist

Added changelog fragment in ./changelog/ (more information)?
~~[ ] Issue raised/referenced?~~

codecov · 2018-02-13T13:45:02Z

Codecov Report

Merging #36 into master will increase coverage by 14.91%.
The diff coverage is 89.65%.

@@             Coverage Diff             @@
##           master      #36       +/-   ##
===========================================
+ Coverage   66.86%   81.78%   +14.91%     
===========================================
  Files          10        8        -2     
  Lines         329      313       -16     
===========================================
+ Hits          220      256       +36     
+ Misses        109       57       -52

Impacted Files	Coverage Δ
mdbenchmark/cli.py	`76.92% <ø> (ø)`	⬆️
mdbenchmark/generate.py	`96.42% <100%> (+7.14%)`	⬆️
mdbenchmark/submit.py	`95.45% <100%> (+62.89%)`	⬆️
mdbenchmark/utils.py	`98.48% <100%> (+16.39%)`	⬆️
mdbenchmark/analyze.py	`38.75% <25%> (+2.5%)`	⬆️
mdbenchmark/ext/cadishi.py
mdbenchmark/ext/click_test.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update baef999...d39cd37. Read the comment docs.

kain88-de · 2018-02-13T13:47:40Z

Makefile

+clean-build:
+	rm --force --recursive build/
+	rm --force --recursive dist/
+	rm --force --recursive *.egg-info


this might delete your developer installation!

kain88-de · 2018-02-13T13:53:17Z

Makefile

+	rm --force --recursive *.egg-info
+
+upload:
+  twine upload dist/*


add a dependency chain here.

.PHONY clean-build build upload clean-build: build: clean-build upload: build

this way we can just say upload and are always sure that the build is clean.

kain88-de · 2018-02-13T13:53:59Z

mdbenchmark/generate.py

-    click.echo('Now run `mdbenchmark start` to submit jobs.')
+    click.echo('Finished generating all benchmarks.')
+    click.echo('You can now submit the jobs with {}.'.format(
+        click.style('mdbenchmark start', bold=True)))


benchmark submit

mimischi · 2018-02-16T15:27:04Z

@kain88-de Besides the missing changelog fragments, this PR should be finished. I have got two questions:

All code in mdbenchmark/ext is untested. Should we write some small tests for this or just tell codecov to ignore that folder? These files are diminishing our overall code coverage.
I changed the exit codes to sys.exit(1), when we exit with an error. I noticed that there is also exit code 2, referring to bad CLI usage. Should we use that when applicable?

mimischi · 2018-02-16T15:46:05Z

Oh, I just noticed a weird edge case with towncrier: if we add several independent features/fix bugs in a PR, we actually need to create several #PR.feature or #PR.bugfix files, but obviously this is not possible.

Putting several changes on multiple lines does also not work in towncrier. What do we do in this case?

* We explicitly test all messages printed to the console errors warnings. * Test the `guess_ncores` utility function.

kain88-de · 2018-02-16T19:48:56Z

What do we do in this case?

Several options.

Notice that you included too many changes in a single PR and split it up
Check the towncrier docs/issues for a solution
ditch towncrier.

kain88-de · 2018-02-16T19:51:39Z

All code in mdbenchmark/ext is untested.

It's not tested anymore in this repo. But the code has been tested before. We only use it as a backup now anyway.

I changed the exit codes to sys.exit(1), when we exit with an error. I noticed that there is also exit code 2, referring to bad CLI usage. Should we use that when applicable?

No just a 1 for error occured. There is no real consistency with exit codes in TUI applications. So let's just stick with 0-OK or 1-Error.

mimischi · 2018-02-19T17:04:10Z

@kain88-de I can't find any other suitable replacement, nor does anyone comment on this issue in the towncrier repository. I would say we just keep it the way it is in this PR, as I just added two short changelog fragmnets.

If you are fine with the PR, we can merge and then I would finally prepare a new release (to finally have the bugfix for mdbenchmark analyze on pip/conda, see #27).

- Added `Makefile` to project. - Updated help text and docstrings for `generate`, `submit` and `analyze`. - Rephrased and formatted messages printed to the console. - `mdbenchmark generate` will now _first_ look for the `.tpr` file and only then do everything else. - Added unit tests for all messages generated via `click.echo()` and `click.BadParameter()`. - Add unit test for `guess_ncores()` utility function.

kain88-de suggested changes Feb 13, 2018

View reviewed changes

mimischi force-pushed the changes branch from 378a6fb to 5d7ecf4 Compare February 13, 2018 15:35

mimischi changed the title ~~[WIP] Minor cleanup~~ [WIP] Cleanup Feb 13, 2018

mimischi force-pushed the changes branch 2 times, most recently from 4403ff0 to 0b3acd3 Compare February 16, 2018 15:25

mimischi added 8 commits February 16, 2018 16:47

Add simple Makefile

5a45998

Update method docstrings and help texts for options.

6e681cc

Rephrase and format messages output to the console.

5f9d22f

Check for existence of .tpr file before doing anything else.

f1e7031

Remove outdated option

9ded983

Change exit codes to 1

9f96f3d

Add more unit tests.

0d99614

* We explicitly test all messages printed to the console errors warnings. * Test the `guess_ncores` utility function.

Remove whitespace from PR template

3e940d2

mimischi force-pushed the changes branch from 4856051 to 3e940d2 Compare February 16, 2018 15:48

Tell coverage to omit ext code folder.

7971171

mimischi force-pushed the changes branch from 89d802e to 7971171 Compare February 18, 2018 09:47

Add changelog files

d39cd37

mimischi changed the title ~~[WIP] Cleanup~~ Update project internals Feb 19, 2018

kain88-de approved these changes Feb 19, 2018

View reviewed changes

mimischi merged commit aeb48d4 into master Feb 20, 2018

mimischi deleted the changes branch February 20, 2018 07:54

mimischi mentioned this pull request Feb 20, 2018

streamline errors and warnings #38

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update project internals #36

Update project internals #36

mimischi commented Feb 13, 2018 •

edited

Loading

codecov bot commented Feb 13, 2018 •

edited

Loading

kain88-de Feb 13, 2018

kain88-de Feb 13, 2018

kain88-de Feb 13, 2018

mimischi commented Feb 16, 2018

mimischi commented Feb 16, 2018

kain88-de commented Feb 16, 2018

kain88-de commented Feb 16, 2018

mimischi commented Feb 19, 2018 •

edited

Loading

Update project internals #36

Update project internals #36

Conversation

mimischi commented Feb 13, 2018 • edited Loading

PR Checklist

codecov bot commented Feb 13, 2018 • edited Loading

Codecov Report

kain88-de Feb 13, 2018

Choose a reason for hiding this comment

kain88-de Feb 13, 2018

Choose a reason for hiding this comment

kain88-de Feb 13, 2018

Choose a reason for hiding this comment

mimischi commented Feb 16, 2018

mimischi commented Feb 16, 2018

kain88-de commented Feb 16, 2018

kain88-de commented Feb 16, 2018

mimischi commented Feb 19, 2018 • edited Loading

mimischi commented Feb 13, 2018 •

edited

Loading

codecov bot commented Feb 13, 2018 •

edited

Loading

mimischi commented Feb 19, 2018 •

edited

Loading