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.

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

Refactorization #1

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

Refactorization #1

wants to merge 6 commits into from

Conversation

cojapacze
Copy link

  • Add .eslintrc with best practices and simply update js code to meet current standards
  • Remove unused functions and other artifacts
  • Change names and reorganize code
  • Add support for asyncBlocking in Chromium with fallback to sync HTTP lookup
  • Use native communication by default

Krzysztof Łukasik added 6 commits June 3, 2019 16:15
Add .eslintrc with best practices and simply update js code to meet current standards
- remove unused resetRequestListener
- Remove currentRequestListener artifact
- Use consts for crucial settings
- Refactor names
- change code order
- Add support for asyncBlocking in chromium with fallback to sync HTTP lookup
- Use native communication by default
- change names from 'compat' to 'unified'
- remove unnecessary functions
- try to keep vars alphabetically ordered
- use compact notation
- remove magic numbers
- use semantic console output
@JeremyRand
Copy link
Member

Thanks for the PR. Generally my policy is that when bugs are fixed, a test should be added so that we don't have regressions later. In this PR's case, it would be useful to have a Bash script (maybe call it contrib/test.sh [1]) that runs eslint using the .eslintrc that you've added. A config file for Travis CI that runs that Bash script would also be great, but isn't required.

[1] We'll probably add more tests to this Bash script later, e.g. things that run stuff in a browser.

@JeremyRand
Copy link
Member

Any particular reason why env doesn't include webextensions? (Looks like HTTPS Everywhere does include it.)

@JeremyRand
Copy link
Member

The HTTPS Everywhere .eslintrc is at https://github.com/EFForg/https-everywhere/blob/master/chromium/.eslintrc.json .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants