Fix Electron linting errors #929
Conversation
IGassmann
added
the
type: improvement
build/install_deps.sh
Outdated
| @@ -42,6 +42,7 @@ if $LINUX; then | |||
| $INSTALL build-essential libssl-dev libffi-dev libgmp3-dev python2.7-dev libsecret-1-dev curl | |||
| elif $OSX && ! cmd_exists brew ; then | |||
| /usr/bin/ruby -e "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/master/install)" | |||
| brew update | |||
There was a problem hiding this comment.
this only gets run if brew command does not exist. so it will only update when it is installed. is that what you intended?
README.md
Outdated
|
|
||
| ### One-time Setup | ||
|
|
||
| 1. Clone this repo | ||
| 2. `DEPS=true ./build.sh` | ||
| 2. `./build.sh` |
There was a problem hiding this comment.
why did you get rid of DEPS=true?
There was a problem hiding this comment.
There's no need anymore for installing dependencies from the build script. The user should install the prerequisites listed in README.md. That is Git, Node, and Yarn. No need anymore for Python for developing.
There was a problem hiding this comment.
it would be much better if users ran a script to install dependencies instead of following a README. that way if the dependencies change, no one has to remember to update the README
| /src/main/locales | ||
| /src/main/node_modules | ||
| /src/renderer/dist | ||
| /build/venv |
There was a problem hiding this comment.
i believe build/venv is still used (by build.sh)
.gitignore
Outdated
| /daemon/build | ||
| /daemon/venv | ||
| /daemon/requirements.txt | ||
| /.idea |
There was a problem hiding this comment.
this is here because many of us use JetBrains, which creates that dir
There was a problem hiding this comment.
I think it's better to keep project related ignore rules into the project .gitignoreand user-specific rules into the user's global .gitignore.
| /daemon/requirements.txt | ||
| /.idea | ||
|
|
||
| *.pyc |
There was a problem hiding this comment.
this has to be here while upload_assets.py is still in use. if you want to rewrite that in JS instead, then we can drop python dependency completely, which would be sweet!
|
|
|
is the app no longer built/released by teamcity? teamcity runs build.sh |
package.json
Outdated
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "name": "LBRY", | |||
| "name": "lbry-app", | |||
There was a problem hiding this comment.
why did this change? will this affect the .deb package that's built? if the name changes, i think ubuntu will think its a different package...
There was a problem hiding this comment.
I changed the name to follow the norms. I'm not sure how exactly it'll behave in Ubuntu since I didn't test on it, but it should normally use the productName field declared in the electron-builder.json file and not the package.json's name.
I'll test it to be sure.
There was a problem hiding this comment.
Debian does identify the app as a new package and doesn't replace the one with the previous name.
I think we should wait for the AppImage switch, which should come with the auto-update PR, before applying this change and then request Linux users that they uninstall their previous install.
This pull request fixes #397 and:
brew updateto build script for getting latest dependencies versions.gitignoreto remove IDE specific ignores and old rules.package-jsonname field to be compliant with npm and yarn rules, and adds productName to buildkeytarwithkeytar-prebuilduntil feat: prebuild binaries for electron and node atom/node-keytar#67 is not merged. This simplifies the build process on Windows. There's no need anymore for Python and Windows building tools.