Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(378)

Issue 29399569: Issue 5060 - Move require into modules template, make info a module (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 9 months ago by kzar
Modified:
2 years, 9 months ago
Reviewers:
Sebastian Noack
Visibility:
Public.

Description

Issue 5060 - Move require into modules template, make info a module

Patch Set 1 #

Patch Set 2 : Don't redeclare require #

Total comments: 9

Patch Set 3 : Addressed some feedback #

Total comments: 7

Patch Set 4 : Addressed more feedback #

Total comments: 7

Patch Set 5 : Improve check for require #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+91 lines, -87 lines) Patch
M packagerChrome.py View 1 2 3 chunks +4 lines, -14 lines 0 comments Download
M packagerEdge.py View 1 chunk +0 lines, -5 lines 0 comments Download
M templates/chromeInfo.js.tmpl View 1 2 3 1 chunk +46 lines, -49 lines 0 comments Download
M templates/geckoInfo.js.tmpl View 1 2 3 1 chunk +14 lines, -18 lines 0 comments Download
M templates/modules.js.tmpl View 1 2 3 4 1 chunk +27 lines, -0 lines 2 comments Download
M tests/test_packagerEdge.py View 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 13
kzar
Patch Set 1
2 years, 9 months ago (2017-03-31 05:46:24 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29399569/diff/29399595/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29399569/diff/29399595/packagerChrome.py#newcode145 packagerChrome.py:145: info_module = None How about merging chromeInfo.js.tmpl, geckoInfo.js.tmpl and ...
2 years, 9 months ago (2017-03-31 10:42:02 UTC) #2
kzar
Patch Set 3 : Addressed some feedback https://codereview.adblockplus.org/29399569/diff/29399595/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29399569/diff/29399595/packagerChrome.py#newcode145 packagerChrome.py:145: info_module = ...
2 years, 9 months ago (2017-03-31 14:03:01 UTC) #3
Sebastian Noack
https://codereview.adblockplus.org/29399569/diff/29399595/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29399569/diff/29399595/packagerChrome.py#newcode145 packagerChrome.py:145: info_module = None On 2017/03/31 14:03:01, kzar wrote: > ...
2 years, 9 months ago (2017-03-31 14:50:45 UTC) #4
kzar
Patch Set 4 : Addressed more feedback https://codereview.adblockplus.org/29399569/diff/29399595/templates/chromeInfo.js.tmpl File templates/chromeInfo.js.tmpl (right): https://codereview.adblockplus.org/29399569/diff/29399595/templates/chromeInfo.js.tmpl#newcode55 templates/chromeInfo.js.tmpl:55: module.exports = ...
2 years, 9 months ago (2017-03-31 15:16:54 UTC) #5
kzar
https://codereview.adblockplus.org/29399569/diff/29399760/templates/modules.js.tmpl File templates/modules.js.tmpl (right): https://codereview.adblockplus.org/29399569/diff/29399760/templates/modules.js.tmpl#newcode4 templates/modules.js.tmpl:4: window.require = function(module) On 2017/03/31 15:16:54, kzar wrote: > ...
2 years, 9 months ago (2017-03-31 15:27:37 UTC) #6
Sebastian Noack
https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.js.tmpl File templates/modules.js.tmpl (left): https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.js.tmpl#oldcode1 templates/modules.js.tmpl:1: {%- if args["module"] -%} It seems we can get ...
2 years, 9 months ago (2017-03-31 16:33:11 UTC) #7
kzar
Patch Set 5 : Improve check for require https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.js.tmpl File templates/modules.js.tmpl (left): https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.js.tmpl#oldcode1 templates/modules.js.tmpl:1: {%- ...
2 years, 9 months ago (2017-04-01 01:32:57 UTC) #8
Sebastian Noack
https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.js.tmpl File templates/modules.js.tmpl (left): https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.js.tmpl#oldcode1 templates/modules.js.tmpl:1: {%- if args["module"] -%} On 2017/04/01 01:32:56, kzar wrote: ...
2 years, 9 months ago (2017-04-01 10:02:56 UTC) #9
kzar
https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.js.tmpl File templates/modules.js.tmpl (left): https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.js.tmpl#oldcode1 templates/modules.js.tmpl:1: {%- if args["module"] -%} On 2017/04/01 10:02:56, Sebastian Noack ...
2 years, 9 months ago (2017-04-01 10:50:29 UTC) #10
Sebastian Noack
https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.js.tmpl File templates/modules.js.tmpl (left): https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.js.tmpl#oldcode1 templates/modules.js.tmpl:1: {%- if args["module"] -%} On 2017/04/01 10:50:28, kzar wrote: ...
2 years, 9 months ago (2017-04-01 11:04:54 UTC) #11
kzar
https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.js.tmpl File templates/modules.js.tmpl (left): https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.js.tmpl#oldcode1 templates/modules.js.tmpl:1: {%- if args["module"] -%} On 2017/04/01 11:04:54, Sebastian Noack ...
2 years, 9 months ago (2017-04-01 12:13:13 UTC) #12
Sebastian Noack
2 years, 9 months ago (2017-04-01 13:09:06 UTC) #13
LGTM

https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.j...
File templates/modules.js.tmpl (left):

https://codereview.adblockplus.org/29399569/diff/29399785/templates/modules.j...
templates/modules.js.tmpl:1: {%- if args["module"] -%}
On 2017/04/01 12:13:13, kzar wrote:
> On 2017/04/01 11:04:54, Sebastian Noack wrote:
> > Merging those scripts, while on it, is trivial. If we do it later, however,
we
> > need to update buildtools, update the buildtools dependency in
> > adblockpluschrome, and adapt adblockpluschrome, again, which is some effort.
> > 
> > Also in regard to that potential no cosmetic changes policy, you voted for,
> the
> > idea was to clean up code while changing it anyway, like we'd do here. Not
> > saying that we will go with that policy, but it seems inconsistent that you
> > agree to it, but then suggest doing the related clean up separately here.
> 
> Honestly I've have had enough of these "while at it" refactoring requests for
a
> while, especially when each one leads to three more. The ESLint stuff was
> painful and it left me fairly burnt out there. I plan to work on the WebRTC
> wrapper next week. I'm happy to try and finish this, or to let someone else
take
> over, but I don't want the scope of the changes to continue to grow if I'm the
> one stuck making them.

Well, you were the one pushing for the ESLint configuration and the related
refactoring. Refactoring the content scripts into modules was necessary, in
order to use the no-undef rule there (which wasn't even my idea) in a sane way,
besides we planned to put everything into modules for a long time anyway. Also
you were the one pushing for dropping Safari support, and despite we are talking
about a change that was just forgotten there, you turn down every opportunity to
correct that. While it was good we did these things, and I'm happy you moved
them forward, I think your reaction here isn't fair.

Generally, I think, it is important while we advance our code, to watch out for
things that can be simplified or cleaned up. Otherwise, we end up in an
unmaintainable mess in the long run (if we aren't there yet), whether the code
is linted or not. Anyway, I guess, I will file a separate issue.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5