Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

install DAP by npm #14

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

install DAP by npm #14

wants to merge 7 commits into from

Conversation

deming7h777
Copy link
Contributor

No description provided.

@hsiaoyi0504 hsiaoyi0504 self-requested a review October 18, 2018 15:34
hsiaoyi0504
hsiaoyi0504 requested changes Oct 18, 2018
View reviewed changes
Copy link
Member

@hsiaoyi0504 hsiaoyi0504 left a comment

Choose a reason for hiding this comment

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

The DAP script should be collected by webpack.

@deming7h777
Copy link
Contributor Author

err packagejson
It's the issue i'm dealing with now

hsiaoyi0504
hsiaoyi0504 requested changes Oct 24, 2018
View reviewed changes
Copy link
Member

@hsiaoyi0504 hsiaoyi0504 left a comment

Choose a reason for hiding this comment

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

See the line I pointed at

package.json Outdated
@@ -44,6 +44,7 @@
"marked": "^0.3.19",
"underscore": "^1.7.0",
"webpack": "^4.20.2",
"webpack-cli": "^3.1.1"
"webpack-cli": "^3.1.1",
"Universal-Federated-Analytics-Min.js": "digital-analytics-program/gov-wide-code#master",
Copy link
Member

Choose a reason for hiding this comment

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

Should not have a trailing comma here.

@hsiaoyi0504
Copy link
Member

Basically, the error message explains itself. It can't not find the package.json. I suspect that it's due to wrong trailing comma. The trailing comma make the json file invalid.

@hsiaoyi0504
Copy link
Member

Don't remove the package-lock.json though.

hsiaoyi0504
hsiaoyi0504 requested changes Oct 24, 2018
View reviewed changes
Copy link
Member

@hsiaoyi0504 hsiaoyi0504 left a comment

Choose a reason for hiding this comment

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

Finally found the correct way ....

package.json Outdated
@@ -44,6 +44,7 @@
"marked": "^0.3.19",
"underscore": "^1.7.0",
"webpack": "^4.20.2",
"webpack-cli": "^3.1.1"
"webpack-cli": "^3.1.1",
"Universal-Federated-Analytics-Min.js": "https://github.com/digital-analytics-program/gov-wide-code/archive/master.zip"
Copy link
Member

Choose a reason for hiding this comment

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

Use "digital-analytics-program": "https://github.com/digital-analytics-program/gov-wide-code.git#fcba3b93059e23b8d1b31df25ac6c86597381395"

@hsiaoyi0504 hsiaoyi0504 changed the title install by npm install DAP by npm Oct 24, 2018
@hsiaoyi0504
Copy link
Member

Need to test if JQuery 3.1 work though. See NAL-i5K/genomics-workspace#249 first.

@deming7h777
Copy link
Contributor Author

i remove package-lock.json just because i want to regenerate the new package-lock.json then, and i still confused why can't i just put #master after the url but for the completed commit.
really thanks for your help!

@hsiaoyi0504
Copy link
Member

The reason is that if we specify the master branch, when they update the master, our application will be directly updated (since we only say we want get master branch of the package). It's ok if we want that update, but if we don't want. That causes the problem.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 125

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.08%) to 50.902%
Files with Coverage Reduction New Missed Lines %
blast/views.py 1 72.32%
util/get_bin_name.py 1 100.0%
Totals Coverage Status
Change from base Build 110: -0.08%
Covered Lines: 1354
Relevant Lines: 2660
💛 - Coveralls

@coveralls
Copy link

coveralls commented Oct 24, 2018 •
edited
Loading

Pull Request Test Coverage Report for Build 129

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 50.977%
Totals Coverage Status
Change from base Build 110: 0.0%
Covered Lines: 1356
Relevant Lines: 2660
💛 - Coveralls

@codecov
Copy link

codecov bot commented Oct 25, 2018 •
edited
Loading

Codecov Report

Merging #14 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #14   +/-   ##
=======================================
  Coverage   50.97%   50.97%           
=======================================
  Files          64       64           
  Lines        2660     2660           
=======================================
  Hits         1356     1356           
  Misses       1304     1304
Impacted Files Coverage Δ
i5k/settings.py 93.02% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68f6ba9...e29f625. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hsiaoyi0504 hsiaoyi0504 hsiaoyi0504 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@deming7h777 @hsiaoyi0504 @coveralls