Issue 29677659: Issue #6303 - Introduce basic foundation for automation in adblockplusui (Closed)
|
Created:
Jan. 23, 2018, 2:42 p.m. by a.giammarchi Modified:
Feb. 13, 2018, 4:35 p.m. Visibility:
Public. |
DescriptionIssue #6303 - Introduce basic foundation for automation in adblockplusui
Patch Set 1 #
Total comments: 21
Patch Set 2 : applied required changes #Patch Set 3 : #
Total comments: 6
Patch Set 4 : #Patch Set 5 : #
Total comments: 7
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Total comments: 2
MessagesTotal messages: 32
I have added http-server to the list of dependencies for the simple reason that is way faster in cross browser testing and also it provides full IP out of the box (needed for tests via VM). The stylelint-config-eyeo package should be installed manually to test it, until #6288 is done (and the package published). This issue is indeed blocked by #6288. P.S. there are JS linting issues solved in other tickets but also many CSS issues that will not be solved in here, as agreed already.
Looking good overall. One comment I'd like to highlight is the one about dependencies. I think it's very important to be proactively transparent about the reasoning behind adding/modifying/removing dependencies to avoid misunderstandings which could lead to bloat and other issues. Especially later on, in case certain third-party code makes it into the extension release, this becomes even more crucial for various reasons such as legal, security and performance. If you two agree with that reasoning, we could later introduce a CONTRIBUTING.md file in which we can mention that among other things we feel are important when making contributions to adblockplusui. On 2018/01/23 14:50:17, a.giammarchi wrote: > I have added http-server to the list of dependencies for the simple reason that > is way faster in cross browser testing and also it provides full IP out of the > box (needed for tests via VM). I'm glad that you removed the Python script because that allows us to more tightly integrate it into the rest of our tooling. https://codereview.adblockplus.org/29677659/diff/29677660/README.md File README.md (right): https://codereview.adblockplus.org/29677659/diff/29677660/README.md#newcode14 README.md:14: Both node dependencies and one python script will run to setup the folder. Detail: I know it's nit-picking but I noticed that you're spelling it "Python" in the very next sentence whereas here you're writing "python" and "node" with lower-case letters. So that inconsistency was a bit too obvious for me not to point out since those two are so close together. :) https://codereview.adblockplus.org/29677659/diff/29677660/README.md#newcode16 README.md:16: You need Python version 2 to ensure a successful `./buildtools` setup. What about mentioning that as part of a list of requirements? Because not only do we require Python 2 but also Node and npm. https://codereview.adblockplus.org/29677659/diff/29677660/README.md#newcode65 README.md:65: You can try as example via `npx stylelint --fix skin/real-file-name.css`. I assume it's a typo but in case it's not: What does "npx" stand for? https://codereview.adblockplus.org/29677659/diff/29677660/package.json File package.json (right): https://codereview.adblockplus.org/29677659/diff/29677660/package.json#newcode3 package.json:3: "version": "0.0.0", Technically, we haven't decided on how we want to version UI releases so we may want to wait for that until specifying a version number. In that regard, setting "private: true" could be appropriate until we have a release process. What do you think? https://codereview.adblockplus.org/29677659/diff/29677660/package.json#newcode4 package.json:4: "description": "AsBlock Plus UI", Typo: Replace "AsBlock Plus UI" with "Adblock Plus UI" BTW we could use the text "Shared Adblock Plus UI code" which we use at https://github.com/adblockplus/adblockplusui and https://hg.adblockplus.org/adblockplusui/ if you want. https://codereview.adblockplus.org/29677659/diff/29677660/package.json#newcode7 package.json:7: "lint:css": "stylelint ./skin", Detail: You're specifying "*.js" for JavaScript linting so is there a reason why you haven't appended "*.css" here? https://codereview.adblockplus.org/29677659/diff/29677660/package.json#newcode20 package.json:20: "eslint-plugin-standard": "^3.0.1", I get that we need both "eslint" and "eslint-config-eyeo" but it'd be great if you could elaborate on why you considered the other ones necessary. While I assume that you got valid reasons for using each of them, I'd like to make sure that we're not including dependencies which we don't actually need. https://codereview.adblockplus.org/29677659/diff/29677660/package.json#newcode21 package.json:21: "http-server": "^0.11.1", Since we're no longer using test_server.py, we should remove that script from the repository.
answered to all the things https://codereview.adblockplus.org/29677659/diff/29677660/README.md File README.md (right): https://codereview.adblockplus.org/29677659/diff/29677660/README.md#newcode16 README.md:16: You need Python version 2 to ensure a successful `./buildtools` setup. On 2018/01/23 19:22:35, Thomas Greiner wrote: > What about mentioning that as part of a list of requirements? Because not only > do we require Python 2 but also Node and npm. Python 2 (or python 2) is available by default in both macOS and Linux (AFAIK including WSL) so I didn't think about it but you are right, it should be as clear as possible that to contribute you need node, npm, and python (so, is python better than Python ? or if Pytno, should I write NodeJS too ?) https://codereview.adblockplus.org/29677659/diff/29677660/README.md#newcode65 README.md:65: You can try as example via `npx stylelint --fix skin/real-file-name.css`. On 2018/01/23 19:22:35, Thomas Greiner wrote: > I assume it's a typo but in case it's not: What does "npx" stand for? npx is the "node package executable" binary that comes with npm 5.2 or higher. https://medium.com/@maybekatz/introducing-npx-an-npm-package-runner-55f7d4bd282b Instead of needing to create a script entry in the package.json file, with a name, and some bash script to pass arguments, and then run it through `npm run executable-module arg1 argn`, you can just `npx executable-module arg1 argn` without needing any entry in package.json. It's a way to quickly use modules with an executable beside the building process. It's a developers oriented shortcut and super handy to avoid global installations. You just npx eslint, or whatever you want, and that's it, ready to test. As summary, I'll put a link to the npx page so others that didn't know npx will learn about it. https://codereview.adblockplus.org/29677659/diff/29677660/package.json File package.json (right): https://codereview.adblockplus.org/29677659/diff/29677660/package.json#newcode3 package.json:3: "version": "0.0.0", On 2018/01/23 19:22:36, Thomas Greiner wrote: > Technically, we haven't decided on how we want to version UI releases so we may > want to wait for that until specifying a version number. In that regard, setting > "private: true" could be appropriate until we have a release process. > > What do you think? makes sense https://codereview.adblockplus.org/29677659/diff/29677660/package.json#newcode4 package.json:4: "description": "AsBlock Plus UI", On 2018/01/23 19:22:35, Thomas Greiner wrote: > Typo: Replace "AsBlock Plus UI" with "Adblock Plus UI" > > BTW we could use the text "Shared Adblock Plus UI code" which we use at > https://github.com/adblockplus/adblockplusui and > https://hg.adblockplus.org/adblockplusui/ if you want. AsBlock ... gosh, 1 word, 2 errors ... coffee here I come again! https://codereview.adblockplus.org/29677659/diff/29677660/package.json#newcode7 package.json:7: "lint:css": "stylelint ./skin", On 2018/01/23 19:22:35, Thomas Greiner wrote: > Detail: You're specifying "*.js" for JavaScript linting so is there a reason why > you haven't appended "*.css" here? skin has no subfolders while eslint was going through the whole project, including node_modules folders. This is why ./*.js is mandatory for eslint, but ./lib and ./ext won't probably need the .js specificity. Accordingly, either I use ./*.js only for the project root to avoid parsing sub folders, and drop *.js for lib and ext, or I add *.css to stylelint command. I don't have strong preferences here, let me know which one you prefer. https://codereview.adblockplus.org/29677659/diff/29677660/package.json#newcode20 package.json:20: "eslint-plugin-standard": "^3.0.1", On 2018/01/23 19:22:35, Thomas Greiner wrote: > I get that we need both "eslint" and "eslint-config-eyeo" but it'd be great if > you could elaborate on why you considered the other ones necessary. > > While I assume that you got valid reasons for using each of them, I'd like to > make sure that we're not including dependencies which we don't actually need. Good point. I've tried `eslint` as command and it incrementally complained about other modules. I start believing something went wrong with the node_folders, parsed as well, probably containing other .eslinrc here and there. I'll try to re-test all the things being sure I always use the ./*.js and lib/ext folders only. https://codereview.adblockplus.org/29677659/diff/29677660/package.json#newcode21 package.json:21: "http-server": "^0.11.1", On 2018/01/23 19:22:36, Thomas Greiner wrote: > Since we're no longer using test_server.py, we should remove that script from > the repository. differently from git, when you commit the lovely mercurial doesn't automatically flag deleted files amnd push them ... yes, I'll be happy to learn how to explicitly drop files via hg.
https://codereview.adblockplus.org/29677659/diff/29677660/README.md File README.md (right): https://codereview.adblockplus.org/29677659/diff/29677660/README.md#newcode16 README.md:16: You need Python version 2 to ensure a successful `./buildtools` setup. On 2018/01/24 10:52:36, a.giammarchi wrote: > Python 2 (or python 2) is available by default in both macOS and Linux (AFAIK > including WSL) so I didn't think about it but you are right, it should be as > clear as possible that to contribute you need node, npm, and python (so, is > python better than Python ? or if Pytno, should I write NodeJS too ?) I guess that's a personal preference so as long as we're consistent with how we refer to them anything should be fine. :) Personally, I tend to check the official homepage/repository to see how they're writing it themselves. https://codereview.adblockplus.org/29677659/diff/29677660/README.md#newcode65 README.md:65: You can try as example via `npx stylelint --fix skin/real-file-name.css`. On 2018/01/24 10:52:36, a.giammarchi wrote: > npx is the "node package executable" binary that comes with npm 5.2 or higher. > https://medium.com/@maybekatz/introducing-npx-an-npm-package-runner-55f7d4bd282b > > Instead of needing to create a script entry in the package.json file, with a > name, and some bash script to pass arguments, and then run it through `npm run > executable-module arg1 argn`, you can just `npx executable-module arg1 argn` > without needing any entry in package.json. > > It's a way to quickly use modules with an executable beside the building > process. It's a developers oriented shortcut and super handy to avoid global > installations. You just npx eslint, or whatever you want, and that's it, ready > to test. I didn't know this was included in npm so thanks for the explanation. > As summary, I'll put a link to the npx page so others that didn't know npx will > learn about it. Sounds good. https://codereview.adblockplus.org/29677659/diff/29677660/package.json File package.json (right): https://codereview.adblockplus.org/29677659/diff/29677660/package.json#newcode7 package.json:7: "lint:css": "stylelint ./skin", On 2018/01/24 10:52:37, a.giammarchi wrote: > On 2018/01/23 19:22:35, Thomas Greiner wrote: > > Detail: You're specifying "*.js" for JavaScript linting so is there a reason > why > > you haven't appended "*.css" here? > > skin has no subfolders while eslint was going through the whole project, > including node_modules folders. This is why ./*.js is mandatory for eslint, but > ./lib and ./ext won't probably need the .js specificity. > > Accordingly, either I use ./*.js only for the project root to avoid parsing sub > folders, and drop *.js for lib and ext, or I add *.css to stylelint command. > > I don't have strong preferences here, let me know which one you prefer. The latter sounds most consistent in that case (i.e. adding "*.css" to the stylelint command) due to the limitation with "./*.js". https://codereview.adblockplus.org/29677659/diff/29677660/package.json#newcode20 package.json:20: "eslint-plugin-standard": "^3.0.1", On 2018/01/24 10:52:36, a.giammarchi wrote: > On 2018/01/23 19:22:35, Thomas Greiner wrote: > > I get that we need both "eslint" and "eslint-config-eyeo" but it'd be great if > > you could elaborate on why you considered the other ones necessary. > > > > While I assume that you got valid reasons for using each of them, I'd like to > > make sure that we're not including dependencies which we don't actually need. > > Good point. > > I've tried `eslint` as command and it incrementally complained about other > modules. I start believing something went wrong with the node_folders, parsed as > well, probably containing other .eslinrc here and there. > > I'll try to re-test all the things being sure I always use the ./*.js and > lib/ext folders only. Thanks, I appreciate that. In case the issue still persists, feel free to share the warnings you get. https://codereview.adblockplus.org/29677659/diff/29677660/package.json#newcode21 package.json:21: "http-server": "^0.11.1", On 2018/01/24 10:52:37, a.giammarchi wrote: > differently from git, when you commit the lovely mercurial doesn't automatically > flag deleted files amnd push them ... yes, I'll be happy to learn how to > explicitly drop files via hg. It's a bit more explicit in that regard, that's true. I think what you're looking for is `hg remove` or `hg addremove`.
applied all required changes. The stylelint-config-eyeo entry in package json still needs for stylelint-config-eyeo to be published in npm. That is quite possibly the only needed extra change before this can land in master.
Great, let's wait until stylelint-config-eyeo is published then. Only added one last comment in regards to "stylelint-config-recommended". https://codereview.adblockplus.org/29677659/diff/29677660/package.json File package.json (right): https://codereview.adblockplus.org/29677659/diff/29677660/package.json#newcode24 package.json:24: "stylelint-config-recommended": "^2.0.1" Since you were able to remove "eslint-config-standard" we can probably also remove "stylelint-config-standard" now unless stylelint works differently in that regard, for some reason.
On 2018/01/24 15:40:36, Thomas Greiner wrote: > Great, let's wait until stylelint-config-eyeo is published then. > > Only added one last comment in regards to "stylelint-config-recommended". > > https://codereview.adblockplus.org/29677659/diff/29677660/package.json > File package.json (right): > > https://codereview.adblockplus.org/29677659/diff/29677660/package.json#newcode24 > package.json:24: "stylelint-config-recommended": "^2.0.1" > Since you were able to remove "eslint-config-standard" we can probably also > remove "stylelint-config-standard" now unless stylelint works differently in > that regard, for some reason. I'm unable to test and validate until stylelint-config-eyeo is published so I will keep this in mind but it makes no sense now to remove it. Why are we waiting for the stylelint-config-eyeo package to be published ? We can always publish a new version when needed, right?
answered
On 2018/01/24 15:45:35, a.giammarchi wrote: > Why are we waiting for the stylelint-config-eyeo package to be published ? We > can always publish a new version when needed, right? Because it's not yet available on npm or are you referring to something different? If we don't want to wait for #6288 we could either exclude CSS linting from this change or somehow else refer to our configuration while it's not on npm yet.
On 2018/01/24 16:08:14, Thomas Greiner wrote: > On 2018/01/24 15:45:35, a.giammarchi wrote: > > Why are we waiting for the stylelint-config-eyeo package to be published ? We > > can always publish a new version when needed, right? > > Because it's not yet available on npm or are you referring to something > different? > > If we don't want to wait for #6288 we could either exclude CSS linting from this > change or somehow else refer to our configuration while it's not on npm yet. I don't think dropping changes to the README and the package.json is convenient, to be honest. I'd rather wait for #6288 to be done, and #5689 before too. Fixing linting issues in master (we have both JS files and CSS with errors) will be done after as separate ticket. Thoughts?
On 2018/01/24 16:24:02, a.giammarchi wrote: > I don't think dropping changes to the README and the package.json is convenient, > to be honest. > > I'd rather wait for #6288 to be done, and #5689 before too. > > Fixing linting issues in master (we have both JS files and CSS with errors) will > be done after as separate ticket. > > Thoughts? That's what we agreed on already anyway so that's my preference too. Note that fixing linting issues has never been part of this review anyway as you noted in your first comment. Your last comment just sounded like you don't want to wait until stylelint-config-eyeo gets published to land this change. But since that's not the case there's no problem.
this should be clean enough and ready to go. Please let me know if there's any outstanding issue, thanks.
ready to go
Just a small things/nits I've noticed and 1 question. https://codereview.adblockplus.org/29677659/diff/29690905/.stylelintrc.json File .stylelintrc.json (right): https://codereview.adblockplus.org/29677659/diff/29690905/.stylelintrc.json#n... .stylelintrc.json:2: "extends": "stylelint-config-eyeo" Nit: We decided to remove stylelint from current review. https://codereview.adblockplus.org/29677659/diff/29690905/package.json File package.json (right): https://codereview.adblockplus.org/29677659/diff/29690905/package.json#newcode6 package.json:6: "lint": "npm run lint:js # keeping :js waiting for :css to land", Question: what does this comment mean ? # keeping :js waiting for :css to land https://codereview.adblockplus.org/29677659/diff/29690905/package.json#newcode9 package.json:9: "start": "http-server $1" Suggestion: Rather than allowing to specify the path, maybe we can allow specifying the port. I'm thinking about something like: "http-server -p $1" Also not sure if we would need caching for the local development, so maybe we can specify: "http-server -p $1 -c-1" The default port for http-server module is 8080, we might want to update the README accordingly: "HTML pages under URLs like `http://127.0.0.1:8080/firstRun.html`" Or you can run `npm start 5000`
https://codereview.adblockplus.org/29677659/diff/29690905/README.md File README.md (right): https://codereview.adblockplus.org/29677659/diff/29690905/README.md#newcode63 README.md:63: You can also run only JS via `npm run lint:js`, or CSS via `npm run lint:css`. Detail: You removed "lint:css" with the latest patch so we need to adapt the README accordingly to avoid confusion. https://codereview.adblockplus.org/29677659/diff/29690905/package.json File package.json (right): https://codereview.adblockplus.org/29677659/diff/29690905/package.json#newcode9 package.json:9: "start": "http-server $1" On 2018/02/06 17:33:23, saroyanm wrote: > Suggestion: Rather than allowing to specify the path, maybe we can allow > specifying the port. I'm thinking about something like: > > "http-server -p $1" > > Also not sure if we would need caching for the local development, so maybe we > can specify: > "http-server -p $1 -c-1" > > The default port for http-server module is 8080, we might want to update the > README accordingly: > "HTML pages under URLs like `http://127.0.0.1:8080/firstRun.html`" > > Or > you can run `npm start 5000` > I'd say let's keep it simple for now and merely replace test_server.py for now. We can still look into caching and other features separately afterwards.
https://codereview.adblockplus.org/29677659/diff/29690905/package.json File package.json (right): https://codereview.adblockplus.org/29677659/diff/29690905/package.json#newcode9 package.json:9: "start": "http-server $1" On 2018/02/06 19:08:48, Thomas Greiner wrote: > On 2018/02/06 17:33:23, saroyanm wrote: > > Suggestion: Rather than allowing to specify the path, maybe we can allow > > specifying the port. I'm thinking about something like: > > > > "http-server -p $1" > > > > Also not sure if we would need caching for the local development, so maybe we > > can specify: > > "http-server -p $1 -c-1" > > > > The default port for http-server module is 8080, we might want to update the > > README accordingly: > > "HTML pages under URLs like `http://127.0.0.1:8080/firstRun.html`" > > > > Or > > you can run `npm start 5000` > > > > I'd say let's keep it simple for now and merely replace test_server.py for now. > We can still look into caching and other features separately afterwards. Okey, in that case we should at least update the README, because it's misleading currently. HTML pages under URLs like `http://127.0.0.1:5000/firstRun.html` to HTML pages under URLs like `http://127.0.0.1:8080/firstRun.html` Or keep the port on 5000
On 2018/02/07 09:07:54, saroyanm wrote: > https://codereview.adblockplus.org/29677659/diff/29690905/package.json > File package.json (right): > > https://codereview.adblockplus.org/29677659/diff/29690905/package.json#newcode9 > package.json:9: "start": "http-server $1" > On 2018/02/06 19:08:48, Thomas Greiner wrote: > > On 2018/02/06 17:33:23, saroyanm wrote: > > > Suggestion: Rather than allowing to specify the path, maybe we can allow > > > specifying the port. I'm thinking about something like: > > > > > > "http-server -p $1" > > > > > > Also not sure if we would need caching for the local development, so maybe > we > > > can specify: > > > "http-server -p $1 -c-1" > > > > > > The default port for http-server module is 8080, we might want to update the > > > README accordingly: > > > "HTML pages under URLs like `http://127.0.0.1:8080/firstRun.html`" > > > > > > Or > > > you can run `npm start 5000` > > > > > > > I'd say let's keep it simple for now and merely replace test_server.py for > now. > > We can still look into caching and other features separately afterwards. > Okey, in that case we should at least update the README, because it's misleading > currently. > HTML pages under URLs like `http://127.0.0.1:5000/firstRun.html` > to > HTML pages under URLs like `http://127.0.0.1:8080/firstRun.html` > Or keep the port on 5000 Good point on updating the README too. Moving to http-server will show the address in the console, and it choses the right port in case the other one is busy so it's an improvement I'd rather keep. npm start is all you need to type, ctrl+click in console to open the url, all ip addresses available for easy mobile testing via IP, if needed, and the ability to pass extra arguments when it's necessary. Deal?
README changes applied + npm start accepts 0 to N extra arguments when/if needed to pass along http-server
https://codereview.adblockplus.org/29677659/diff/29691567/README.md File README.md (right): https://codereview.adblockplus.org/29677659/diff/29691567/README.md#newcode63 README.md:63: You can also run only JS via `npm run lint:js`, or CSS via `npm run lint:css`. Nit: We don't have CSS linting yet in the package https://codereview.adblockplus.org/29677659/diff/29691567/package.json File package.json (right): https://codereview.adblockplus.org/29677659/diff/29691567/package.json#newcode9 package.json:9: "start": "http-server \"${@:2}\"" I'm getting an error "sh: 1: Bad substitution" when running `npm start`
On 2018/02/07 11:48:27, saroyanm wrote: > https://codereview.adblockplus.org/29677659/diff/29691567/README.md > File README.md (right): > > https://codereview.adblockplus.org/29677659/diff/29691567/README.md#newcode63 > README.md:63: You can also run only JS via `npm run lint:js`, or CSS via `npm > run lint:css`. > Nit: We don't have CSS linting yet in the package > > https://codereview.adblockplus.org/29677659/diff/29691567/package.json > File package.json (right): > > https://codereview.adblockplus.org/29677659/diff/29691567/package.json#newcode9 > package.json:9: "start": "http-server \"${@:2}\"" > I'm getting an error "sh: 1: Bad substitution" > > when running `npm start` which Bash version are you?
https://codereview.adblockplus.org/29677659/diff/29691567/package.json File package.json (right): https://codereview.adblockplus.org/29677659/diff/29691567/package.json#newcode9 package.json:9: "start": "http-server \"${@:2}\"" On 2018/02/07 11:48:26, saroyanm wrote: > I'm getting an error "sh: 1: Bad substitution" > > when running `npm start` Bash version -> "4.3.11(1)-release (x86_64-pc-linux-gnu)"
On 2018/02/07 12:08:16, saroyanm wrote: > https://codereview.adblockplus.org/29677659/diff/29691567/package.json > File package.json (right): > > https://codereview.adblockplus.org/29677659/diff/29691567/package.json#newcode9 > package.json:9: "start": "http-server \"${@:2}\"" > On 2018/02/07 11:48:26, saroyanm wrote: > > I'm getting an error "sh: 1: Bad substitution" > > > > when running `npm start` > > Bash version -> "4.3.11(1)-release (x86_64-pc-linux-gnu)" do you mind updating your system? Thomas is on LTS and its version is 4.3.38 and has zero issues, while I'm on 4.4.18 in macOS and still zero issue. sudo apt-get update && sudo apt-get install --only-upgrade bash should be sufficient. Meanwhile, I've changed the README to be less misleading about CSS.
Sorry were having Technical issues after update, had to update bash manually, but still same issue. @Andrea, can you please comment inline in the code please, when posssible, otherwise I'm having hard time to find the replys. https://codereview.adblockplus.org/29677659/diff/29691567/package.json File package.json (right): https://codereview.adblockplus.org/29677659/diff/29691567/package.json#newcode9 package.json:9: "start": "http-server \"${@:2}\"" On 2018/02/07 12:08:16, saroyanm wrote: > On 2018/02/07 11:48:26, saroyanm wrote: > > I'm getting an error "sh: 1: Bad substitution" > > > > when running `npm start` > > Bash version -> "4.3.11(1)-release (x86_64-pc-linux-gnu)" I've updated to 4.4.0(1)-release but still same issue, can you please confirm that @Thomas can you please confirm that you are using the latest patch ?
On 2018/02/07 14:02:04, saroyanm wrote: > Sorry were having Technical issues after update, had to update bash manually, > but still same issue. > > @Andrea, can you please comment inline in the code please, when posssible, > otherwise I'm having hard time to find the replys. I'm replying directly from the trac page, I guess that's the issue. I'll try to follow links and reply inline.
https://codereview.adblockplus.org/29677659/diff/29691567/package.json File package.json (right): https://codereview.adblockplus.org/29677659/diff/29691567/package.json#newcode9 package.json:9: "start": "http-server \"${@:2}\"" On 2018/02/07 14:02:04, saroyanm wrote: > On 2018/02/07 12:08:16, saroyanm wrote: > > On 2018/02/07 11:48:26, saroyanm wrote: > > > I'm getting an error "sh: 1: Bad substitution" > > > > > > when running `npm start` > > > > Bash version -> "4.3.11(1)-release (x86_64-pc-linux-gnu)" > > I've updated to 4.4.0(1)-release but still same issue, can you please confirm > that @Thomas can you please confirm that you are using the latest patch ? Note: "start": "http-server '${@:2}'" Fixes the issue for me.
https://codereview.adblockplus.org/29677659/diff/29691567/package.json File package.json (right): https://codereview.adblockplus.org/29677659/diff/29691567/package.json#newcode9 package.json:9: "start": "http-server \"${@:2}\"" On 2018/02/07 14:21:41, saroyanm wrote: > On 2018/02/07 14:02:04, saroyanm wrote: > > On 2018/02/07 12:08:16, saroyanm wrote: > > > On 2018/02/07 11:48:26, saroyanm wrote: > > > > I'm getting an error "sh: 1: Bad substitution" > > > > > > > > when running `npm start` > > > > > > Bash version -> "4.3.11(1)-release (x86_64-pc-linux-gnu)" > > > > I've updated to 4.4.0(1)-release but still same issue, can you please confirm > > that @Thomas can you please confirm that you are using the latest patch ? > > Note: "start": "http-server '${@:2}'" > Fixes the issue for me. OK this is an issue with npm using sh, not bash, and sh in macOS has backslashes inconsistencies compared to Linux. The "force bash" way to go is to pass: bash -c \"http-server ${@:2}\" but this becomes an ugly indirection of just http-server Thomas suggested we just pass along arguments through -- as in: npm start -- -p 5000 which is fine. I am updating script and documentation about this. Sorry for the inconvenience.
https://codereview.adblockplus.org/29677659/diff/29691567/package.json File package.json (right): https://codereview.adblockplus.org/29677659/diff/29691567/package.json#newcode9 package.json:9: "start": "http-server \"${@:2}\"" On 2018/02/07 14:30:04, a.giammarchi wrote: > On 2018/02/07 14:21:41, saroyanm wrote: > > On 2018/02/07 14:02:04, saroyanm wrote: > > > On 2018/02/07 12:08:16, saroyanm wrote: > > > > On 2018/02/07 11:48:26, saroyanm wrote: > > > > > I'm getting an error "sh: 1: Bad substitution" > > > > > > > > > > when running `npm start` > > > > > > > > Bash version -> "4.3.11(1)-release (x86_64-pc-linux-gnu)" > > > > > > I've updated to 4.4.0(1)-release but still same issue, can you please > confirm > > > that @Thomas can you please confirm that you are using the latest patch ? > > > > Note: "start": "http-server '${@:2}'" > > Fixes the issue for me. > > OK this is an issue with npm using sh, not bash, and sh in macOS has backslashes > inconsistencies compared to Linux. > > The "force bash" way to go is to pass: > bash -c \"http-server ${@:2}\" > but this becomes an ugly indirection of just http-server > > Thomas suggested we just pass along arguments through -- as in: > > npm start -- -p 5000 > > which is fine. > > I am updating script and documentation about this. > Sorry for the inconvenience. Sounds good, thanks.
ready for review
All looks great to me, Happy that we are finally moving forward with the automation. LGTM
LGTM I added a suggestion so I'll leave it up to you whether or not you want to address it. https://codereview.adblockplus.org/29677659/diff/29691610/README.md File README.md (right): https://codereview.adblockplus.org/29677659/diff/29691610/README.md#newcode12 README.md:12: Both [python 2](https://www.python.org/downloads/) and [node](https://nodejs.org/en/), as well as [npm](https://www.npmjs.com), are required to contribute to this repository. Suggestion: We don't have a coding style for Markdown but I noticed some arbitrary line breaks in this file so what about sticking to the 80 characters line length that all the other text in this file conforms to?
answered https://codereview.adblockplus.org/29677659/diff/29691610/README.md File README.md (right): https://codereview.adblockplus.org/29677659/diff/29691610/README.md#newcode12 README.md:12: Both [python 2](https://www.python.org/downloads/) and [node](https://nodejs.org/en/), as well as [npm](https://www.npmjs.com), are required to contribute to this repository. On 2018/02/12 13:01:43, Thomas Greiner wrote: > Suggestion: We don't have a coding style for Markdown but I noticed some > arbitrary line breaks in this file so what about sticking to the 80 characters > line length that all the other text in this file conforms to? agreed but this has been already merged so I'll keep that in mind for the next time. Does it sound reasonable?
Message was sent while issue was closed.
On 2018/02/12 14:52:55, a.giammarchi wrote: > agreed but this has been already merged so I'll keep that in mind for the next > time. Does it sound reasonable? Yep, that's fine. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
