Issue 29531680: Issue 5560 - Specify the JSDoc dependency explicitly (Closed)
DescriptionIssue 5560 - Specify the JSDoc dependency explicitly
Patch Set 1 #
Total comments: 7
Patch Set 2 : Addressed nit #
Total comments: 2
Patch Set 3 : Readdressed nit #MessagesTotal messages: 12
Patch Set 1
Hey Dave! Looks pretty good, just one tiny nit from my side ;) Best, Tristan https://codereview.adblockplus.org/29531680/diff/29531681/build.py File build.py (right): https://codereview.adblockplus.org/29531680/diff/29531681/build.py#newcode522 build.py:522: command.description = 'Generate documentation files and write them into the specified directory.' nit: While at it, how do you feel about limiting line length to 79 characters? (PEP-8)
https://codereview.adblockplus.org/29531680/diff/29531681/package-lock.json File package-lock.json (right): https://codereview.adblockplus.org/29531680/diff/29531681/package-lock.json#n... package-lock.json:1: { I wonder whether we should add package-lock.json to the repository, or rather list it in .hgignore/.gitignore. While the official documentation recommends to commit this file to your repository, I'm not convinced that freezing the whole environment, pinning the version of each module in the dependency tree, is worth it. Not to mention the additional boilerplate, and effort to keep all (indirect) dependencies up-to-date.
https://codereview.adblockplus.org/29531680/diff/29531681/package-lock.json File package-lock.json (right): https://codereview.adblockplus.org/29531680/diff/29531681/package-lock.json#n... package-lock.json:1: { On 2017/08/30 18:36:24, Sebastian Noack wrote: > While the official documentation recommends to > commit this file to your repository, I'm not convinced that freezing the whole > environment, pinning the version of each module in the dependency tree, is worth > it. Initially, I wasn't sure either. However, you have to consider this: if we don't pin the versions, we cannot really know which version buildtools users will install. They might get a newer version of a module which introduces a subtle incompatibility and results in a broken build. It's better to pin a known-good state that we'll update explicitly when we feel like it, in the interest of reproducible builds.
https://codereview.adblockplus.org/29531680/diff/29531681/package-lock.json File package-lock.json (right): https://codereview.adblockplus.org/29531680/diff/29531681/package-lock.json#n... package-lock.json:1: { On 2017/08/30 18:50:01, Wladimir Palant wrote: > On 2017/08/30 18:36:24, Sebastian Noack wrote: > > While the official documentation recommends to > > commit this file to your repository, I'm not convinced that freezing the whole > > environment, pinning the version of each module in the dependency tree, is > worth > > it. > > Initially, I wasn't sure either. However, you have to consider this: if we don't > pin the versions, we cannot really know which version buildtools users will > install. They might get a newer version of a module which introduces a subtle > incompatibility and results in a broken build. It's better to pin a known-good > state that we'll update explicitly when we feel like it, in the interest of > reproducible builds. I wouldn't mind pinning the version of JSDoc and webpack in package.json. However, this is about pinning each of their dependencies as well. If JSDoc or webpack require a specific version of a package this is already be handled in their package.json file. Also note that we don't pin any or our Python dependencies either. So even if in theory a minor version difference of a dependency of webpack could result in slightly different builds, we still have the same issue with our Python dependencies.
https://codereview.adblockplus.org/29531680/diff/29531681/package-lock.json File package-lock.json (right): https://codereview.adblockplus.org/29531680/diff/29531681/package-lock.json#n... package-lock.json:1: { On 2017/08/30 19:02:33, Sebastian Noack wrote: > On 2017/08/30 18:50:01, Wladimir Palant wrote: > > On 2017/08/30 18:36:24, Sebastian Noack wrote: > > > While the official documentation recommends to > > > commit this file to your repository, I'm not convinced that freezing the > whole > > > environment, pinning the version of each module in the dependency tree, is > > worth > > > it. > > > > Initially, I wasn't sure either. However, you have to consider this: if we > don't > > pin the versions, we cannot really know which version buildtools users will > > install. They might get a newer version of a module which introduces a subtle > > incompatibility and results in a broken build. It's better to pin a known-good > > state that we'll update explicitly when we feel like it, in the interest of > > reproducible builds. > > I wouldn't mind pinning the version of JSDoc and webpack in package.json. > However, this is about pinning each of their dependencies as well. If JSDoc or > webpack require a specific version of a package this is already be handled in > their package.json file. Also note that we don't pin any or our Python > dependencies either. So even if in theory a minor version difference of a > dependency of webpack could result in slightly different builds, we still have > the same issue with our Python dependencies. Imho, the thing to consider is that a packages' package.json (which is beyond our control) allows for non-pinned versions - that way, in theory, a major version difference is also perfectly possible. While quickly following down the current dependencies, jsdoc's package.json allows patch updates of e.g. klaw, while klaw itself allows minor version updates for e.g. graceful-fs. I'm not sure, however, if and in which way a Node.js environment tends to break on updates of any degree. Also imho, amending an autogenerated file is well worth the effort, when we get a reproducible environment in return.
On 2017/08/30 19:02:33, Sebastian Noack wrote: > I wouldn't mind pinning the version of JSDoc and webpack in package.json. > However, this is about pinning each of their dependencies as well. Yes, they matter as well. And while it's a large file in our repository, it is also autogenerated. I wouldn't mind pinning Python dependencies as well but there is no easy way. npm provides this mechanism and we should use it.
Patch Set 2 : Addressed nit https://codereview.adblockplus.org/29531680/diff/29531681/build.py File build.py (right): https://codereview.adblockplus.org/29531680/diff/29531681/build.py#newcode522 build.py:522: command.description = 'Generate documentation files and write them into the specified directory.' On 2017/08/30 17:33:21, tlucas wrote: > nit: While at it, how do you feel about limiting line length to 79 characters? > (PEP-8) Done. https://codereview.adblockplus.org/29531680/diff/29531681/package-lock.json File package-lock.json (right): https://codereview.adblockplus.org/29531680/diff/29531681/package-lock.json#n... package-lock.json:1: { On 2017/08/30 20:49:47, tlucas wrote: > On 2017/08/30 19:02:33, Sebastian Noack wrote: > > On 2017/08/30 18:50:01, Wladimir Palant wrote: > > > On 2017/08/30 18:36:24, Sebastian Noack wrote: > > > > While the official documentation recommends to > > > > commit this file to your repository, I'm not convinced that freezing the > > whole > > > > environment, pinning the version of each module in the dependency tree, is > > > worth > > > > it. > > > > > > Initially, I wasn't sure either. However, you have to consider this: if we > > don't > > > pin the versions, we cannot really know which version buildtools users will > > > install. They might get a newer version of a module which introduces a > subtle > > > incompatibility and results in a broken build. It's better to pin a > known-good > > > state that we'll update explicitly when we feel like it, in the interest of > > > reproducible builds. > > > > I wouldn't mind pinning the version of JSDoc and webpack in package.json. > > However, this is about pinning each of their dependencies as well. If JSDoc or > > webpack require a specific version of a package this is already be handled in > > their package.json file. Also note that we don't pin any or our Python > > dependencies either. So even if in theory a minor version difference of a > > dependency of webpack could result in slightly different builds, we still have > > the same issue with our Python dependencies. > > Imho, the thing to consider is that a packages' package.json (which is beyond > our control) allows for non-pinned versions - that way, in theory, a major > version difference is also perfectly possible. > > While quickly following down the current dependencies, jsdoc's package.json > allows patch updates of e.g. klaw, while klaw itself allows minor version > updates for e.g. graceful-fs. I'm not sure, however, if and in which way a > Node.js environment tends to break on updates of any degree. > > Also imho, amending an autogenerated file is well worth the effort, when we get > a reproducible environment in return. FWIW I am in favour of pinning dependencies where possible as well.
LGTM
Hey dave, almost perfect - please see below. Cheers! https://codereview.adblockplus.org/29531680/diff/29532591/build.py File build.py (right): https://codereview.adblockplus.org/29531680/diff/29532591/build.py#newcode523 build.py:523: 'the specified directory.' Sorry for bothering, PEP 8 prefers implicit continuation with parentheses over using a backslash, e.g.: command.description = ('Generate documentation files and write them into ' 'the specified directory.')
Patch Set 3 : Readdressed nit https://codereview.adblockplus.org/29531680/diff/29532591/build.py File build.py (right): https://codereview.adblockplus.org/29531680/diff/29532591/build.py#newcode523 build.py:523: 'the specified directory.' On 2017/08/31 12:20:58, tlucas wrote: > Sorry for bothering, PEP 8 prefers implicit continuation with parentheses over > using a backslash, e.g.: > > command.description = ('Generate documentation files and write them into ' > 'the specified directory.') Done.
LGTM 
|
