Added --format #23

erayerdin · 2018-10-29T08:04:02Z

Related to #20

Added --format flag (for short, -f) to cli args. --json is kept for convenience.

  -j, --json            Output JSON. Shortcut for "-f json".
  -f FORMAT, --format FORMAT
                        The format of output. Supported: json, markdown.
                        Default is "markdown".

The default is markdown. The cli tool also provides a UserWarning if given format is not supported and will inform user that it will fallback to default, which is, again, markdown. An example below:

$ pypistats recent pyairmore -f mp3  # siriously, wth?
foo/bar/pypistats/pypistats/cli.py:132: UserWarning: Unknown format: mp3. Using "markdown".
  warnings.warn(f'Unknown format: {args.format}. Using "markdown".')
| last_day | last_month | last_week |
|---------:|-----------:|----------:|
|        3 |        151 |        33 |

Other Minor Changes

Verbose flake8 and black output on Travis
flake8 and black now checks pypistats/ and tests/ directory instead of whole project which makes it a little bit faster (especially black).
Added tests_require to setup.py.

The only problem is that the code I provided was not tested by pytest since it is not quite possible to both mock HTTP requests and test cli at the same time. The codebase was tested by hand 😢. This will probably drop code coverage by ~5%. If you are strict about testing, I can find a way around (then inform me, don't merge this PR).

If you'd like to test for yourself by hand, you can clone forked copy and checkout feat/format-arg branch.

…inor, system | closes #20

codecov · 2018-10-29T08:14:04Z

Codecov Report

Merging #23 into master will increase coverage by 3.52%.
The diff coverage is 92.3%.

@@            Coverage Diff            @@
##           master     #23      +/-   ##
=========================================
+ Coverage   78.97%   82.5%   +3.52%     
=========================================
  Files           3       3              
  Lines         195     200       +5     
=========================================
+ Hits          154     165      +11     
+ Misses         41      35       -6

Impacted Files	Coverage Δ
pypistats/cli.py	`83.78% <92.3%> (+9.87%)`	⬆️

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 b11a8c7...a62484a. Read the comment docs.

erayerdin · 2018-10-29T08:15:50Z

Wow, that's too much. I think I have to find a way to test cli. Do not merge.

…

On Mon, Oct 29, 2018, 11:14 codecov[bot] ***@***.*** wrote: Codecov <https://codecov.io/gh/hugovk/pypistats/pull/23?src=pr&el=h1> Report Merging #23 <https://codecov.io/gh/hugovk/pypistats/pull/23?src=pr&el=desc> into master <https://codecov.io/gh/hugovk/pypistats/commit/b11a8c7ba05e2e8979f5a98a88537ca04bc87a12?src=pr&el=desc> will *decrease* coverage by 11.87%. The diff coverage is 2.7%. [image: Impacted file tree graph] <https://codecov.io/gh/hugovk/pypistats/pull/23?src=pr&el=tree> @@ Coverage Diff @@## master #23 +/- ## ===========================================- Coverage 78.97% 67.09% -11.88% =========================================== Files 3 3 Lines 195 231 +36 ===========================================+ Hits 154 155 +1 - Misses 41 76 +35 Impacted Files <https://codecov.io/gh/hugovk/pypistats/pull/23?src=pr&el=tree> Coverage Δ pypistats/cli.py <https://codecov.io/gh/hugovk/pypistats/pull/23/diff?src=pr&el=tree#diff-cHlwaXN0YXRzL2NsaS5weQ==> 49.03% <0%> (-24.88%) ⬇️ pypistats/__init__.py <https://codecov.io/gh/hugovk/pypistats/pull/23/diff?src=pr&el=tree#diff-cHlwaXN0YXRzL19faW5pdF9fLnB5> 81.74% <100%> (+0.14%) ⬆️ ------------------------------ Continue to review full report at Codecov <https://codecov.io/gh/hugovk/pypistats/pull/23?src=pr&el=continue>. *Legend* - Click here to learn more <https://docs.codecov.io/docs/codecov-delta> Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov <https://codecov.io/gh/hugovk/pypistats/pull/23?src=pr&el=footer>. Last update b11a8c7...b98dbf3 <https://codecov.io/gh/hugovk/pypistats/pull/23?src=pr&el=lastupdated>. Read the comment docs <https://docs.codecov.io/docs/pull-request-comments> . — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#23 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACSbbCGhXyltLId_6zOBKSHq71c8sIDPks5uprjMgaJpZM4X-pYn> .

hugovk · 2018-10-29T09:41:24Z

This block is repeated five times, and could go into a function which would reduce the coverage drop, and would be more easily tested:

    if args.json or args.format == "json":
        output = "json"
    elif args.format == "markdown":
        output = "table"
    else:
        import warnings

        warnings.warn(f'Unknown format: {args.format}. Using "markdown".')
        output = "table"

That may sort the coverage drop, otherwise some options for testing is using requests_mock like in 4459141, or perhaps even mocking pypistats.recent.

hugovk

A few other notes :)

hugovk · 2018-10-29T09:42:00Z

pypistats/cli.py

+    elif args.format == "markdown":
+        output = "table"
+    else:
+        import warnings


Let's move the import to the top and use isort to put them in order.

hugovk · 2018-10-29T09:44:27Z

.travis.yml

- - flake8 --statistics --count
- - if [[ $TRAVIS_PYTHON_VERSION == 3.6 ]]; then black --check --diff .; fi
+ - flake8 --statistics --count --verbose pypistats/ tests/
+ - if [[ $TRAVIS_PYTHON_VERSION == 3.6 ]]; then black --check --diff --verbose pypistats/ tests/; fi


Is there a reason to specify these dirs and not run in .? We miss setup.py like this.

Also, let's leave out --verbose so the output is a cleaner.

I didn't think about setup.py, sorry.

I put --verbose in case of referring Travis build logs via URL if anyone wants to contribute to show contributors what the problem is.

Sorry, just read your PR message fully now, and it's done for speed.

The CI took 3.13s to run Black with this PR:

https://travis-ci.org/hugovk/pypistats/jobs/447664623#L640

and 4.54s with master:

https://travis-ci.org/hugovk/pypistats/jobs/447539626#L580

I think that's fine.

If there's a flake8 error, it lists them in the logs wihout --verbose.

And if Black errors, --diff shows what should be have been done.

I think the verbose stuff is probably a bit distracting.

pypistats/cli.py

setup.py

hugovk · 2018-10-29T09:49:04Z

pypistats/__init__.py

@@ -15,6 +15,8 @@

 BASE_URL = "https://pypistats.org/api/"

+FORMATS = ("json", "markdown")  # only used for printing to terminal


Would this be better kept internal to cli.py?

I'll do it.

erayerdin · 2018-10-29T10:03:31Z

This block is repeated five times, and could go into a function which would reduce the coverage drop...

Yes, that's a good idea.

...some options for testing is using requests_mock like in 4459141, or perhaps even mocking pypistats.recent.

I was just trying requests_mock. On the other hand, I thought about two methods to test cli with requests_mock alongside.

Method 1: Using Subprocess

I can test via subprocess also using requests_mock.

Pros	Cons
Can provide a basis for behavioral(ish?) tests for cli	Since `subprocess` spawns another process, `requests_mock` will not be able to mock.
	Because `subprocess` is testing cli output, it will not drastically change the coverage.

Method 2: Testing Functions under `cli.py`

I could test @subcommand decorated functions under cli.py.

Pros	Cons
Simple enough	Testing `print` is not possible.

What I'm Going to Do

Overall, I think, as you've said, I will move those repeating if-else parts to a protected/underscore function and see the coverage myself. Then choose a way to solve the problem.

And, also, will look into the problems you've mentioned above. :)

erayerdin · 2018-10-29T17:45:50Z

Phew, finally.

Properly tested, covered. Dealt with minor issues you have mentioned.

into feat/format-arg

hugovk

Looking good, a few more notes :)

pypistats/cli.py

hugovk · 2018-10-29T18:12:21Z

pypistats/cli.py

+            default="markdown",
+            choices=FORMATS,
+            help="The format of output.",
+        ),


Let's put all these repeated argument(...)s in an arg_format variable, like arg_start_date

👍 will do

hugovk · 2018-10-29T18:12:57Z

pypistats/cli.py

+            "--format",
+            default="markdown",
+            choices=FORMATS,
+            help="The format of output.",


Remove fullstop for single sentence:

Suggested change

help="The format of output.",

help="The format of output",

👍 will do

hugovk · 2018-10-29T18:34:39Z

pypistats/cli.py

    if args.mirrors in ["with", "without"]:
        args.mirrors = args.mirrors == "with"
+
+    _format = _define_format(args)


We can actually move all these calls down to a single args.format = _define_format(args) down in the else block at the end of the file, and then use output=_format here.

👍 will check

hugovk · 2018-10-29T20:05:53Z

One more thing, one of the two hardest things in computer science, and it's not cache invalidation. :)

First, we're still at version 0.y.z so I'm not too worried about backwards compatibility at this point, if it means getting it right.

So on the programmatic side, we have output which can be json or table:

print(pypistats.recent("pillow"))  # output="table"
print(pypistats.recent("pillow", output="table"))
print(pypistats.recent("pillow", output="json"))

Previously on the CLI side we had --json (which gave output=json) or a default (output=table) which was Markdown.

$ pypistats recent pillow         # output="table"
$ pypistats recent pillow --json  # output="json"

Now on the CLI side, we have --format which can be json or markdown. When the user makes a CLI call, we translate format -> output and markdown -> table.

$ pypistats recent pillow                    # output="table"
$ pypistats recent pillow --format markdown  # output="table"
$ pypistats recent pillow --json             # output="json"
$ pypistats recent pillow --format json      # output="json"

What do you think, shall we unify the terms? It would probably clearer if this tool isn't using different terms in its two interfaces (CLI and programmatic).

For example, use either output or format on both sides, and table or markdown on both sides:

$ pypistats recent pillow                    # output="markdown"
$ pypistats recent pillow --output markdown  # output="markdown"
$ pypistats recent pillow --json             # output="json"
$ pypistats recent pillow --output json      # output="json"

output has the benefit of not shadowing the builtin format (I'm also open to a third choice)
markdown is more specific than table especially when RST and HTML tables are added
and markdown is the default anyway.

Thoughts? (This could go in a separate PR.)

erayerdin · 2018-10-29T20:42:51Z

TL;DR

format is the way to go.
All hail markdown.
can provide contribution on a separate PR
All these are just my humble opinions. If you are okay, then so am I.

format vs. output

Shadowing Issue

format definitely shadows Python's built-in method. However, it naturally does not shadow if it is a property of an object. In our case in cli.py, we mostly use args.format (in my latest commit at least) rather than format alone. So, IMHO, it is not much of an issue.

On the programmatic side, *insert developer here* provides format as a method parameter, which clearly does not cause any shadowing. I mean:

pypistats.recent("pillow", format="json")  # if a developer does this
format(whatever)   # they can still use built-in `format` method

Terms

Yes, format and output are interchangeable terms, yet I think they sense different.

I do not know if it makes sense, but markdown, rst, html and json are formats since they have convenient rules. On the other hand, output, to me at least, is like a binary mimetype, like an Excel file or Word file, which use different formats like xmls and xhtmls and then zip the final product altogether. If we had plans to provide detailed Excel output on cli in order to create a file, which we probably do not for now, then we could use output right then and there.

Is semantics or terminology worth the hassle though? This is the issue. I think we should use the term format and update the codebase, which is hard but not a pain in the butt. I use Pycharm Professional. It has generous reformatting capabilities, which will make it easier and then run tests locally and test by hand to ensure anything works.

On markdown vs. table and Backwards Compability

...we're still at version 0.y.z so I'm not too worried about backwards compatibility at this point...

I actually wished to change it, but, you know, hesitated it since you might want to be comfortable in your terms. If backwards compatibility is not a huge deal now, I can deal with that. My two cents are on "markdown".

…ain` function

hugovk · 2018-10-29T20:56:35Z

pypistats/cli.py

 arg_daily = argument("-d", "--daily", action="store_true", help="Show daily downloads")
+arg_format = argument(
+    "-f", "--format", default="markdown", choices=FORMATS, help="The format of output."


Suggested change

"-f", "--format", default="markdown", choices=FORMATS, help="The format of output."

"-f", "--format", default="markdown", choices=FORMATS, help="The format of output"

lol I was dead sure I have deleted it, seems like I didn't.

pypistats/cli.py

hugovk · 2018-10-29T21:23:47Z

Yes, let's go for markdown (in another PR).

If this output was format, would it shadow the builtin?

pypistats/pypistats/__init__.py

Lines 19 to 27 in 1ee8418

    
           def pypi_stats_api( 
        
               endpoint, 
        
               params=None, 
        
               output="table", 
        
               start_date=None, 
        
               end_date=None, 
        
               sort=True, 
        
               total=True, 
        
           ):

pypistats/pypistats/__init__.py

Lines 48 to 49 in 1ee8418

    
           if output == "json": 
        
               return json.dumps(res)

pypistats/cli.py

erayerdin · 2018-10-29T21:33:52Z

If this output was format, would it shadow the builtin?

It shadows format in pypi_stats_api method's scope, but not out of it. In programmatic sense, it won't affect the developer who uses this library. In terms of our codebase, it might, depending on if we decide to use format inside pypi_stats_api method in the future.

If shadowing format inside inside pypi_status_api method is a significant issue, then we can keep output as is, throwing away format.

hugovk · 2018-10-29T21:49:55Z

Let's go with format and markdown. If we need to use the builtin in the future, we can always do something like del format. Or just use f-strings instead :)

hugovk · 2018-10-30T11:55:18Z

Thank you very much!

erayerdin added 6 commits October 29, 2018 01:50

added tests_require

783d667

added --format and -f flag to recent, overall, python_major, python_m…

e7259cf

…inor, system | closes #20

updated flake8 and black on travis

03dbe49

blackified, updated travis

711bd3a

blackified pypistats/__init__.py

dfb9364

added black to tests_require

b98dbf3

hugovk reviewed Oct 29, 2018

View reviewed changes

erayerdin added 6 commits October 29, 2018 18:56

added cli._define_format method and used in @subcommands

effe522

black formatting

f3922e6

moved FORMATS to cli.py

0cc6d6b

reverted flake and black scripts on

f9f9f5d

sorted alphabetically

97a3774

no cover pragma added to printing functions for coverage

3557032

erayerdin added 2 commits October 29, 2018 20:48

no cover pragma added to printing functions for coverage | closes #20

ec82640

Merge branch 'feat/format-arg' of https://github.com/erayerdin/pypistats

2e5bbd0

into feat/format-arg

hugovk reviewed Oct 29, 2018

View reviewed changes

erayerdin added 2 commits October 29, 2018 22:58

arg moved to variable

7281281

arg_json help updated

3a8b076

erayerdin added 2 commits October 29, 2018 23:44

_format variables reformatted to args.format and moved down to `m…

6b243af

…ain` function

reformatted for black

93faf81

hugovk reviewed Oct 29, 2018

View reviewed changes

pypistats/cli.py Show resolved Hide resolved

removed dot on arg_format

28188a7

reordered arg_format and arg_json together on subcommand decorators

a62484a

hugovk merged commit 2b6e89e into hugovk:master Oct 30, 2018

erayerdin mentioned this pull request Oct 30, 2018

Change "table" to "markdown" in codebase #24

Closed

erayerdin deleted the feat/format-arg branch October 31, 2018 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added --format #23

Added --format #23

erayerdin commented Oct 29, 2018

codecov bot commented Oct 29, 2018 •

edited

Loading

erayerdin commented Oct 29, 2018 via email

hugovk commented Oct 29, 2018

hugovk left a comment

hugovk Oct 29, 2018

erayerdin Oct 29, 2018

hugovk Oct 29, 2018

erayerdin Oct 29, 2018

hugovk Oct 29, 2018

hugovk Oct 29, 2018

hugovk Oct 29, 2018

erayerdin Oct 29, 2018

erayerdin commented Oct 29, 2018

erayerdin commented Oct 29, 2018

hugovk left a comment

hugovk Oct 29, 2018

erayerdin Oct 29, 2018

hugovk Oct 29, 2018

erayerdin Oct 29, 2018

hugovk Oct 29, 2018

erayerdin Oct 29, 2018

hugovk commented Oct 29, 2018

erayerdin commented Oct 29, 2018 •

edited

Loading

hugovk Oct 29, 2018

erayerdin Oct 29, 2018

hugovk commented Oct 29, 2018

erayerdin commented Oct 29, 2018

hugovk commented Oct 29, 2018

hugovk commented Oct 30, 2018

		@@ -15,6 +15,8 @@

		BASE_URL = "https://pypistats.org/api/"

		FORMATS = ("json", "markdown") # only used for printing to terminal

	"-f", "--format", default="markdown", choices=FORMATS, help="The format of output."
	"-f", "--format", default="markdown", choices=FORMATS, help="The format of output"

Added --format #23

Added --format #23

Conversation

erayerdin commented Oct 29, 2018

Other Minor Changes

codecov bot commented Oct 29, 2018 • edited Loading

Codecov Report

erayerdin commented Oct 29, 2018 via email

hugovk commented Oct 29, 2018

hugovk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erayerdin commented Oct 29, 2018

Method 1: Using Subprocess

Method 2: Testing Functions under cli.py

What I'm Going to Do

erayerdin commented Oct 29, 2018

hugovk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hugovk commented Oct 29, 2018

erayerdin commented Oct 29, 2018 • edited Loading

TL;DR

format vs. output

Shadowing Issue

Terms

On markdown vs. table and Backwards Compability

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hugovk commented Oct 29, 2018

erayerdin commented Oct 29, 2018

hugovk commented Oct 29, 2018

hugovk commented Oct 30, 2018

codecov bot commented Oct 29, 2018 •

edited

Loading

Method 2: Testing Functions under `cli.py`

erayerdin commented Oct 29, 2018 •

edited

Loading