[pypi] Add badge for django version support #1286

chris48s · 2017-11-27T19:52:57Z

This PR adds the django version support feature suggested in issue #1018

Having worked on this issue, I note that:

There don't seem to be tests for the existing PyPi badges
All the PyPi badges are defined in one big function (I am aware this PR makes this situation worse, not better)

As a further piece of work, I would also be interested in adding more tests to support refactoring this into a number of smaller functions and also having a go at some of that refactoring work. Would you be open to receiving some further PRs?

Closes #1018

paulmelnikow · 2017-11-27T23:04:45Z

Hi, thanks for this! Here's a quick response to your question:

As a further piece of work, I would also be interested in adding more tests to support refactoring this into a number of smaller functions and also having a go at some of that refactoring work. Would you be open to receiving some further PRs?

We've got an impending rewrite of all the service code, so yes! Tests are basically half the work of the rewrite, so full steam ahead on those. As far as major refactoring, it's probably best to hold off until the rewrite. If it's a matter of extracting a couple helper functions as we prepare for the rewrite, definitely, feel free. Helpers should get their own unit tests.

The reason to avoid major refactoring now is that it'll cause duplication of work coding and reviewing. Your time and the maintainers' time is better spent writing more tests, and putting in place the rest of the groundwork for the rewrite.

chris48s · 2017-11-28T19:43:49Z

OK. Thanks for the extra info @paulmelnikow . I'll look at adding some tests but hold off on refactoring.

One observation I have about this project is that a lot of of your tests depend on external services. i.e: the tests may start fail because of something that has changed outside of your codebase. This also seems to be causing some other problems. Again, I'm aware that the tests I've added here have the same problem due to the structure.

Given you're planning to refactor a lot of this code, would I be right to assume one of the things you're hoping to do is to decouple the code that reaches out to external services from the code that processes the responses? That would make it easier to test the code against mocked responses which would make the tests more deterministic and help with some of these other issues.

I realise I've strayed a bit from the original point of adding a badge to display django version support. If it is better to reply under another issue, feel free to move the conversation elsewhere and @ mention me.

chris48s · 2017-11-29T20:53:06Z

I've rebased this on to master and tweaked the tests to account for your feedback on #1289

paulmelnikow

I left you a few comments.

paulmelnikow · 2017-11-29T23:35:24Z

lib/all-badge-examples.js

@@ -1299,6 +1299,14 @@ const allBadgeExamples = [
          'python'
        ]
      },
+      {
+        title: 'PyPI',


The title should be PyPI – Django version or maybe just Django version, and it's probably worth adding pypi to keywords.

I've named it consistently with all the other PyPI badges:

Shall I update the others to "PyPI - License", "PyPI - Format" and so on as well?

Sure! That sounds great.

paulmelnikow · 2017-11-29T23:36:43Z

server.js

@@ -2094,7 +2094,7 @@ cache(function(data, match, sendBadge, request) {
    }
    try {
      var parsedData = JSON.parse(buffer);
-      if (info.charAt(0) === 'd') {
+      if ((info === 'dm') || (info === 'dw') || (info ==='dd')) {


You can omit the parens around these, since || binds at a lower precedence. Everywhere but server.js I think the linter will catch it.

paulmelnikow · 2017-11-29T23:38:09Z

server.js

+        if (!versions.length) {
+          versions.push('not found');
+        }
+


Could you extract some the version parsing and formatting into a helper function in lib/pypi-helpers.js?

paulmelnikow · 2017-11-29T23:39:06Z

service-tests/pypi.js

@@ -4,6 +4,7 @@ const Joi = require('joi');
 const ServiceTester = require('./runner/service-tester');
 const { isSemver } = require('./helpers/validators');
 const isCommaSeperatedPythonVersions = Joi.string().regex(/^([0-9]+.[0-9]+[,]?[ ]?)+$/);
+const isCommaSeperatedDjangoVersions = Joi.string().regex(/^([0-9]+.[0-9]+[,]?[ ]?)+$/);


These regexes are the same. 🤔

Yes - they're the same regex being used to validate 2 different things which is why I've assigned that regex to 2 different names. I did think about this myself and thought about:

const isCommaSeperatedPythonVersions = Joi.string().regex(/^([0-9]+.[0-9]+[,]?[ ]?)+$/); const isCommaSeperatedDjangoVersions = isCommaSeperatedPythonVersions;

or

const isCommaSeperatedPythonOrDjangoVersions = Joi.string().regex(/^([0-9]+.[0-9]+[,]?[ ]?)+$/);

but I think the way I've written it make it most clear. There's no meaningful performance consideration here so it is all about readability. Do you prefer a different notation?

Sure, makes sense. Maybe just throw in a comment with that rationale?

// These regexes are the same, but defined separately for clarity.

paulmelnikow

This is looking great!

paulmelnikow · 2017-12-01T03:28:54Z

lib/pypi-helpers.js

+
+
+// extract classifiers from a pypi json response based on a regex
+const parseClassifiers = function(parsedData, pattern) {


Could I ask you to write unit tests for these functions? There are some nice data-driven tests in lib/lua-rocks-version.spec.js, among others.

paulmelnikow · 2017-12-01T03:29:30Z

server.js

+        let versions = parseClassifiers(
+          parsedData,
+          /^Programming Language :: Python :: ([\d.]+)$/
+        );


chris48s · 2017-12-01T20:51:47Z

added

paulmelnikow · 2017-12-01T21:09:49Z

lib/pypi-helpers.spec.js

+
+    given(["2.0rc1", "10", "1.9", "1.11", "2.1", "2.11",])
+      .expect(["1.9", "1.11", "2.0rc1", "2.1", "2.11", "10"]);
+  });


paulmelnikow added the service-badge New or updated service badge label Nov 27, 2017

chris48s mentioned this pull request Nov 28, 2017

[pypi] add test suite for PyPi service #1289

Merged

chris48s added 2 commits November 29, 2017 20:39

add badge for supported django versions (issue badges#1018)

fcce1a2

add django versions badge to examples

241130f

chris48s force-pushed the django-versions branch 2 times, most recently from 7ddaac4 to 818c7c4 Compare November 29, 2017 20:45

add service tests for pypi django versions

ccc12a7

chris48s force-pushed the django-versions branch from 818c7c4 to ccc12a7 Compare November 29, 2017 20:48

paulmelnikow mentioned this pull request Nov 29, 2017

Service testing strategy #927

Closed

paulmelnikow reviewed Nov 29, 2017

View reviewed changes

chris48s added 5 commits November 30, 2017 19:16

remove unnecessary brackets

fabc351

facor out parsing and sorting code to a helper

e0e3dc8

factor out code for parsing pypi classifiers

7c97ed5

rename pypi badges, add pypi to keywords

33fafbe

clarify regex declarations

7a0f5c6

paulmelnikow reviewed Dec 1, 2017

View reviewed changes

add unit tests for pypi helpers

5c03c8b

paulmelnikow reviewed Dec 1, 2017

View reviewed changes

paulmelnikow merged commit 56b82e1 into badges:master Dec 1, 2017

paulmelnikow mentioned this pull request Dec 6, 2017

Badge for modules that come with TypeScript type definitions #1252

Closed

snyk-bot mentioned this pull request Apr 7, 2021

[Snyk] Upgrade ioredis from 4.17.3 to 4.24.2 pmutale/shields#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pypi] Add badge for django version support #1286

[pypi] Add badge for django version support #1286

chris48s commented Nov 27, 2017

paulmelnikow commented Nov 27, 2017

chris48s commented Nov 28, 2017

chris48s commented Nov 29, 2017

paulmelnikow left a comment

paulmelnikow Nov 29, 2017

chris48s Nov 30, 2017

paulmelnikow Nov 30, 2017

paulmelnikow Nov 29, 2017

paulmelnikow Nov 29, 2017

paulmelnikow Nov 29, 2017

chris48s Nov 30, 2017

paulmelnikow Nov 30, 2017

paulmelnikow left a comment

paulmelnikow Dec 1, 2017

paulmelnikow Dec 1, 2017

chris48s commented Dec 1, 2017

paulmelnikow Dec 1, 2017



		// extract classifiers from a pypi json response based on a regex
		const parseClassifiers = function(parsedData, pattern) {

[pypi] Add badge for django version support #1286

[pypi] Add badge for django version support #1286

Conversation

chris48s commented Nov 27, 2017

paulmelnikow commented Nov 27, 2017

chris48s commented Nov 28, 2017

chris48s commented Nov 29, 2017

paulmelnikow 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

paulmelnikow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris48s commented Dec 1, 2017

Choose a reason for hiding this comment