user support for updating JS #83

rohitner · 2018-03-08T18:35:40Z

I have used the OptionParser as mentioned in #47

Shekharrajak · 2018-03-09T06:34:29Z

Please write few examples/description/proper commit msg to understand how it will work along with PR. It will be better if we create different commands for updating the different library js files. i.e. (1 command for updating the highcharts related files another command for updating the googlecharts related files). Means I want it to be modular and extendable. The main command (which will update all the js files) will use these commands.

Refer this One command must do only one thing.

athityakumar · 2018-03-09T06:55:16Z

bin/daru-view

+    opts.on('-u', '--update', 'Update js libraries') do
+	path = `gem which daru/view`.split('/')
+	path = path - [path[-1]]
+	path = path.join('/')


path = `gem which daru/view`.split('/')[0...-1].join('/')

rohitner · 2018-03-09T15:11:57Z

Usage

To get help do :
daru-view or daru-view --help
To update JS files for google charts do:
daru-view update -g or daru-view update --googlecharts
To update JS files for highcharts do:
daru-view update -H or daru-view --highcharts

rohitner · 2018-03-09T15:28:47Z

Should I add the following to prevent rubocop fail because replacing __FILE__ with __dir__ in the files mentioned in offenses leads to failing rspec.

Style/ExpandPathArguments:
  Enabled: false

Shekharrajak

Can you write test case for it. I hope there must be a way to test it.

rohitner · 2018-03-11T14:23:30Z

I have managed to prevent failing build without altering the tests. Please check.

v0dro · 2018-03-21T01:42:31Z

@Shekharrajak can you review this change and see if its good enough for merging?

Shekharrajak · 2018-03-22T06:16:44Z

I will review it by tonight. I was waiting for testcases .

Shekharrajak · 2018-03-24T08:05:47Z

bin/daru-view

+  opts.banner = "Usage: daru-view [options]"
+  # Update google charts javascript dependent files, from latest Builds on google developers website
+  opts.on('-g', '--update-googlecharts', 'Update googlecharts.js libraries') do
+    sh "curl -# http://www.google.com/jsapi -L --compressed -o #{path}/view/adapters/js/googlecharts_js/google_visualr.js"


It must create folder and file if it is not present, right ?

Yes, will make that change.

What if file is also not present or deleted to update it ? touch can be useful here, right ?

I thought about this too. curl downloads the missing/deleted files. I have checked it by manually removing some js files. So it won't be a problem.

Even I tried deleting the files. But it was throwing error. I hope you can write testcases for it. Please write in comment the TODO part as well.

Updated the travis script to run the executable and added the TODO.

Shekharrajak · 2018-03-24T08:07:19Z

bin/daru-view

+
+  # Update highcharts.js from latest Builds on Highcharts codebase: http://code.highcharts.com/
+  opts.on('-H', '--update-highcharts', 'Update highcharts.js libraries') do
+    sh "mkdir -p #{path}/view/adapters/js/highcharts_js/modules/"


We must have modularity , to update just modules or just adapters or just particular file.

I have a doubt, why will someone using daru-view care about updating adapters and modules separately or a specific file? I think it is a different issue of having updates in a granular manner.

For example, if someone wants to use new features added in highcharts3d then user may just want to update the file highcharts-3d.js . Let me know if it is little tricky task. We will add it as TODO task .

It will be better to add it as a TODO task in #47 as it will require a bit of restructuring. This PR will make it easy to atleast update the JS files for now. ☺️

Shekharrajak · 2018-03-24T08:10:56Z

The idea of the PR looks good, please have a look on comments .

Shekharrajak · 2018-04-02T11:37:24Z

.travis.yml

@@ -11,6 +11,8 @@ rvm:
 script:
  - bundle exec rspec
  - bundle exec rubocop
+  - ./bin/daru-view -g
+  - ./bin/daru-view -H


Can we have testcases like this : https://github.com/bundler/bundler/blob/master/spec/commands/info_spec.rb ?

Yes, I have added tests in RSpec instead of Travis, please check.

athityakumar · 2018-05-05T13:43:12Z

spec/update_spec.rb

@@ -0,0 +1,22 @@
+describe Daru::View, "Update JS files" do


Change the tab indent to 2 spaces. 😄

athityakumar · 2018-05-05T13:43:28Z

spec/update_spec.rb

+			expect(flag).to eq(true)
+		end
+	end
+end


Add an empty line here.

Shekharrajak · 2018-05-20T10:24:22Z

bin/daru-view

@@ -3,10 +3,12 @@
 require 'optparse'
 require 'rake'

-path = `gem which daru/view`.split('/')[0...-1].join('/')


Your commit message is saying something else !

Let me know why did you change this.

It seemed to be an overkill for getting the path, So I just made it simpler by using the relative path. Pardon for the commit message. 😅

Shekharrajak

I have tested the command in isolated container and docker it is working fine.

I was waiting for the spec which will test the files (i.e. whether js file are created/updated or not). Also some modular way of doing the task.

Merging it for now. Thanks @rohitner !

Shekharrajak · 2018-05-20T18:25:54Z

@rohitner , Can you please document it ?

Shekharrajak · 2018-05-22T05:13:32Z

bin/daru-view

+  opts.banner = "Usage: daru-view [options]"
+  # TODO add arguments to update only modules/adapters/a particular file
+
+  # Update google charts javascript dependent files, from latest Builds on google developers website


Please maintain line length everywhere in the PR.

Shekharrajak · 2018-05-22T05:14:44Z

spec/update_spec.rb

@@ -0,0 +1,22 @@
+describe Daru::View, "Update JS files" do
+  context "googlecharts" do


Shekharrajak · 2018-05-22T05:15:25Z

spec/update_spec.rb

+    end
+  end
+
+  context "googlecharts" do


Shekharrajak · 2018-05-22T09:59:41Z

bin/daru-view

+  opts.on('-g', '--update-googlecharts', 'Update googlecharts.js libraries') do
+    sh "mkdir -p #{path}/view/adapters/js/googlecharts_js/"
+
+    sh "curl -# http://www.google.com/jsapi -L --compressed -o #{path}/view/adapters/js/googlecharts_js/google_visualr.js"


Line length can be reduced,right ?

Shekharrajak · 2018-05-22T10:00:05Z

bin/daru-view

+    sh "mkdir -p #{path}/view/adapters/js/highcharts_js/modules/"
+    sh "mkdir -p #{path}/view/adapters/js/highcharts_js/adapters/"
+
+    sh "curl -# http://code.highcharts.com/highcharts.js -L --compressed -o #{path}/view/adapters/js/highcharts_js/highcharts.js"


Can't we make them in a loop ?

rohitner · 2018-05-22T11:15:37Z

To keep everything DRY, I have replaced the code block with rake tasks.

Shekharrajak · 2018-05-23T06:43:35Z

@rohitner , I will test it once more in isolated (docker) container. Let me know if you have tested all the commands properly in your system.

rohitner · 2018-05-23T08:11:05Z

Actually there's a problem, we always need a rakefile in current directory for this to work. I will revert back to previous commit if there aren't any ways to tackle this.

rohitner · 2018-05-25T20:53:00Z

@Shekharrajak , sorry for delay, please review

rohitner · 2018-06-01T06:48:02Z

ping! @Shekharrajak

Shekharrajak

Good work by using the already present rake task.

TBH, thor is better if we want to test it properly, WDYT?

Shekharrajak · 2018-06-02T03:30:55Z

bin/daru-view

+  # Update google charts javascript dependent files, from latest Builds
+  # on the google developers website
+  opts.on('-g', '--update-googlecharts', 'Update googlecharts.js libraries') do
+    update(:googlecharts)


create a folder googlecharts_js, if it is not present. Better place will be in rake task.

Shekharrajak · 2018-06-02T03:31:30Z

bin/daru-view

+    update(:highcharts)
+  end
+
+  opts.on('-h', '--help', 'Displays Help') do


How it will handle if invalid option is given?

Shekharrajak · 2018-06-04T19:03:01Z

@rohitner , I just wanted to know your opinion. Let me know if you have checked it properly. This PR must be merged soon. Remaining thing we will do .

rohitner · 2018-06-04T19:41:16Z

Hey @Shekharrajak , I actually saw the need of thor gem as I had to define a seperate update method in previous commit. I have checked it and everything works fine. This gem will be perfect for future work of providing granular updates.

Shekharrajak · 2018-06-05T07:17:56Z

Thanks @rohitner ! I had tested few commands in isolated container it worked fine. I still have few concerns :

If invalid command is typed then it must display the help command only. When help command is typed then all the commands must be displayed.
I think along with this daru-view help update, we must also have daru-view help update -g, daru-view help update -h. If -g is lowercase then for highcharts as well it should be lowercase -h. For help we may have -H.
Tescases need to be improved (we need to check the updated/present js file or some other kind of specs which can validate it is not breaking).

I am putting these into TODO list.

Let me know @rohitner , if you have any suggestions/opinions. I will merge it after that.

rohitner · 2018-06-05T08:12:42Z

IMO, we should stick to the conventional -h for help command (change googlecharts to -G maybe). Regarding tests, we can run the update spec followed by remaining tests to check if the updated/present js files work properly. Good to go! ☺️

Shekharrajak · 2018-06-05T13:59:28Z

change googlecharts to -G

will be better I guess.

Merging it for now.

rohitner force-pushed the cmd-update-js branch from cff5570 to 3149475 Compare March 8, 2018 18:43

athityakumar reviewed Mar 9, 2018

View reviewed changes

add exe

be233e1

rohitner force-pushed the cmd-update-js branch from 3149475 to be233e1 Compare March 9, 2018 15:13

Shekharrajak reviewed Mar 11, 2018

View reviewed changes

fixed rubocop error

c98ad03

Shekharrajak reviewed Mar 24, 2018

View reviewed changes

add command mkdir

063f6d3

rohitner force-pushed the cmd-update-js branch 5 times, most recently from 65c3481 to e45721b Compare March 30, 2018 21:03

added tests and TODO

593feb2

rohitner force-pushed the cmd-update-js branch from e45721b to 593feb2 Compare March 31, 2018 09:47

Shekharrajak reviewed Apr 2, 2018

View reviewed changes

rohitner force-pushed the cmd-update-js branch from 313fdac to a05d261 Compare May 1, 2018 10:17

rohitner added 2 commits May 1, 2018 15:48

resolved conflict

a05d261

shift update test from travis to rspec

ed81bff

athityakumar reviewed May 5, 2018

View reviewed changes

spec/update_spec.rb

@@ -0,0 +1,22 @@

describe Daru::View, "Update JS files" do

Copy link

Member

athityakumar May 5, 2018

Choose a reason for hiding this comment

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

Change the tab indent to 2 spaces. 😄

athityakumar reviewed May 5, 2018

View reviewed changes

spec/update_spec.rb Outdated

expect(flag).to eq(true)

end

end

end

Copy link

Member

athityakumar May 5, 2018

Choose a reason for hiding this comment

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

Add an empty line here.

remove typo

38b7b4c

Shekharrajak reviewed May 20, 2018

View reviewed changes

Shekharrajak approved these changes May 20, 2018

View reviewed changes

update README

be08e6e

Shekharrajak reviewed May 22, 2018

View reviewed changes

fix minor typos

394916d

Shekharrajak reviewed May 22, 2018

View reviewed changes

invoke rake task from exe

82cf48d

fix rake error

c018303

Shekharrajak reviewed Jun 2, 2018

View reviewed changes

add thor gem

c6adbac

Shekharrajak merged commit 67c6df6 into SciRuby:master Jun 5, 2018

Shekharrajak mentioned this pull request Jun 5, 2018

Update JS files for the libraries #47

Open

Shekharrajak mentioned this pull request Jan 19, 2019

Reduce the size of the gem #127

Open

		@@ -0,0 +1,22 @@
		describe Daru::View, "Update JS files" do
		context "googlecharts" do

user support for updating JS #83

user support for updating JS #83

Conversation

rohitner commented Mar 8, 2018 • edited Loading

Shekharrajak commented Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

rohitner commented Mar 9, 2018 • edited Loading

Usage

rohitner commented Mar 9, 2018 • edited Loading

Shekharrajak left a comment

Choose a reason for hiding this comment

rohitner commented Mar 11, 2018 • edited Loading

v0dro commented Mar 21, 2018

Shekharrajak commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shekharrajak commented Mar 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohitner May 20, 2018 • edited Loading

Choose a reason for hiding this comment

Shekharrajak left a comment

Choose a reason for hiding this comment

Shekharrajak commented May 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohitner commented May 22, 2018

Shekharrajak commented May 23, 2018

rohitner commented May 23, 2018

rohitner commented May 25, 2018

rohitner commented Jun 1, 2018

Shekharrajak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shekharrajak commented Jun 4, 2018

rohitner commented Jun 4, 2018 • edited Loading

Shekharrajak commented Jun 5, 2018

rohitner commented Jun 5, 2018

Shekharrajak commented Jun 5, 2018

rohitner commented Mar 8, 2018 •

edited

Loading

Shekharrajak commented Mar 9, 2018 •

edited

Loading

rohitner commented Mar 9, 2018 •

edited

Loading

rohitner commented Mar 9, 2018 •

edited

Loading

rohitner commented Mar 11, 2018 •

edited

Loading

rohitner May 20, 2018 •

edited

Loading

rohitner commented Jun 4, 2018 •

edited

Loading