Skip to content
This repository has been archived by the owner on Apr 2, 2019. It is now read-only.

Update to service-template-node v0.5.4 #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mdholloway
Copy link
Contributor

@mdholloway mdholloway commented Jul 4, 2018

Brings in changes to the service template since 0.3.0, which appears to be
the last update incorporated here.

@mdholloway
Copy link
Contributor Author

mdholloway commented Jul 5, 2018

Not for merging yet...

Notes to self:

  • Restore bunyan-prettystream
  • Figure out why this breaks tileratorui

(The latter was because of added (and now removed) CSP rules, which are not needed b/c tileratorui is an admin interface not exposed to the public.)

Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

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

hi, thanks for working on this. I don't think its a good idea to change eslint rules just for the sake of changing. To be honest, the whole template-based approach goes against every good coding practice, because it relies on manually merging new version of the code line by line, at a huge time expense and code instability, instead of relying on a well established API between libraries providing some services, and their consumer. Templates should be used once as a starting point, and never merged again. That said, I do understand why you want to migrate to the newer template version. But lets try to keep changes to the mininum, otherwise we will have a huge code churn, without being able to use history very productively. You may want to exclude template-supplied files from eslint to keep them "in the original state", but lets keep all Kartotherian code in the same style. Thanks!

@mdholloway
Copy link
Contributor Author

The advantages of changing to eslint-config-node-services, as I see them, are (1) improving consistency with other node services maintained in whole or in part by WMF, and (2) making it easier to pull in template updates when we want to do so in the future. That said, I'm fine sticking with the current ESLint rules, too. Will update.

@mdholloway mdholloway force-pushed the tpl/update branch 2 times, most recently from 251f406 to 87472ee Compare July 5, 2018 14:40
@mdholloway mdholloway removed the wip label Jul 5, 2018
@mdholloway
Copy link
Contributor Author

OK, this is now ready for review. The Kartotherian ESLint config is restored and the remaining changes are substantive.

@mdholloway
Copy link
Contributor Author

(Side note: I agree that the template update process is clumsy. To the extent there are common utilities that need updating from time to time, those should probably go into an imported module.)

@mdholloway
Copy link
Contributor Author

@nyurik Sorry, was the change you requested to restore the Kartotherian ESLint config, or not to do a template-upgrade commit at all? There is a bit more work I could do to trim this one down (like restoring the Grunt stuff) but I won't bother if the PR is DOA in any case.

This was really meant as more of a hygiene-type patch and not as high in priority as stuff like https://phabricator.wikimedia.org/T177019 / #34.

@nyurik
Copy link
Member

nyurik commented Jul 13, 2018

@mdholloway I would like to get the original eslint config for this project, but you may want to exclude files from eslinting that are mostly copy/paste from the template.

Brings in changes to the service template since 0.3.0, which appears to be
the last update incorporated here.
@mdholloway
Copy link
Contributor Author

Back to kartotherian eslint config, backed out the Grunt removal, added .eslintignore for files from the template.

@nyurik
Copy link
Member

nyurik commented Apr 2, 2019

Please resubmit to https://github.com/kartotherian/kartotherian

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants