Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split various utilities into standalone files #952

Merged
merged 3 commits into from
Apr 24, 2017

Conversation

Daniel15
Copy link
Member

@Daniel15 Daniel15 commented Apr 20, 2017

The following utilities have been split into other files:

  • php-version.js
    • phpStableVersion (renamed to "isStable")
    • phpLatestVersion (renamed to "latest")
    • phpVersionCompare (renamed to "compare")
    • phpNumberedVersionData (renamed to "numberedVersionData")
    • Only used within PHP version comparison code:
      • omitv
      • asciiVersionCompare
  • version.js
    • latestVersion (renamed to "latest")
    • listCompare
    • latestDottedVersion
    • compareDottedVersions
  • color-formatters.js:
    • versionColor (renamed to "version")
    • floorCountColor (renamed to "floorCount")
    • downloadCountColor (renamed to "downloadCount")
    • coveragePercentageColor (renamed to "coveragePercentage")
  • text-formatters.js
    • starRating
    • currencyFromCode
    • ordinalNumber
    • metric

To avoid changing any of the code in server.js that uses these methods, I'm aliasing them when importing:

const {
  compare: phpVersionCompare,
  latest: phpLatestVersion,
  isStable: phpStableVersion,
} = require('./lib/php-version.js');

(this syntax is object destructuring, added in Node.js 6.0.0)

This allows all code using phpVersionCompare() to continue to work with no changes and will avoid merge conflicts in the open pull requests.

Ideally I would have done something like this:

const phpVersion = require('./lib/php-version.js');

and then changed phpVersionCompare() to phpVersion.compare(), to keep everything namespaced(-ish).

I confirmed that these changes worked on Node.js 6.4.0 by running the server and then loading try.html.

There's a few badges that are broken that aren't due to this pull requests - For example the example badge for "Scrutinizer branch" shows "NaN%" 😛

References #948
cc @paulmelnikow

Copy link
Member

@paulmelnikow paulmelnikow left a comment

Choose a reason for hiding this comment

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

Looks good!

*/
'use strict';

const {listCompare} = require('./version.js');
Copy link
Member

@paulmelnikow paulmelnikow Apr 20, 2017

Choose a reason for hiding this comment

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

You can omit the .js suffix in these requires. There are a few in server.js too.

downloadCount: downloadCountColor,
floorCount: floorCountColor,
version: versionColor,
} = require('./lib/color-formatters.js');
Copy link
Member

Choose a reason for hiding this comment

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

As you wrote, this seems like a good way to handle this, for the moment, to avoid conflicts. It'll be easy to change these names later.

* including colours based off download count, version number, etc.
*/
'use strict';

Copy link
Member

Choose a reason for hiding this comment

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

Did you make any changes in these implementations?

@paulmelnikow
Copy link
Member

Thanks for the great explanation and the changes.

Will leave this open a couple days in case @espadrine wants to comment.

I'll be AFK this weekend BTW. ⛰ 🛶 🏕

@Daniel15
Copy link
Member Author

Daniel15 commented Apr 20, 2017 via email

@paulmelnikow
Copy link
Member

I used the .js suffix for consistency, because the other requires are using it.

Hehe, didn't realize that. When I see one, I wonder if something strange is going on with those files. We may as well stop, don't you think?

@paulmelnikow paulmelnikow merged commit ccbdad6 into badges:master Apr 24, 2017
@paulmelnikow paulmelnikow added this to the Next Deploy milestone Apr 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Dev tooling, test framework, and CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants