Skip to content

feat(bridge): support primitive adbPath expansion#3

Merged
mpotthoff merged 3 commits intompotthoff:masterfrom
buschtoens:patch-1
Aug 9, 2021
Merged

feat(bridge): support primitive adbPath expansion#3
mpotthoff merged 3 commits intompotthoff:masterfrom
buschtoens:patch-1

Conversation

@buschtoens
Copy link
Copy Markdown
Contributor

In some setups, the adb may not be in the $PATH, but be located relative to the workspace directory or accessible via some other environment $VAR. This is currently not supported via the adbPath configuration.

This PR adds support for primitive Bash-like path / variable expansion for adbPath.

  • ~/adb/Users/<user>/adb
    Replace leading ~/ with user home directory. Does not support other Bash-isms, like ~name or ~1.
  • ./adb<workspace>/adb
    Replace leading ./ with primary workspace directory.
  • $HOME/adb/Users/<user>/adb
    Substitute $VAR with environment variables.
  • ../tools/adb<workspace>/../adb
    Resolve .. directory traversal.

Thanks your your great extension!

@mpotthoff
Copy link
Copy Markdown
Owner

Well thank you for your great contribution! It looks really good so far. 😄

Sorry about the failing build. That's also partly my fault because the project still uses typescript 3.9.4 and I haven't taken the time to upgrade it to the latest and greatest yet 🙈 I will try to get everything up-to-date later today so that I can get your changes in.

@mpotthoff mpotthoff self-assigned this Aug 5, 2021
@mpotthoff
Copy link
Copy Markdown
Owner

Okay, so I updated the tooling to the latest TypeScript version and also switched from TSLint to ESLint. It would be great if you could rebase your changes on my latest commit and fix any linter and compiler errors so that I can merge your changes. 😊

Support primitive path / variable expansion for `adbPath`.

- Replace leading `~/` with user home directory.
  `~/adb` → `/Users/<user>/adb`
  Does _not_ support other bash-isms, like `~name` or `~1`.
- Replace leading `./` with primary workspace directory.
  `./adb` → `<workspace>/adb`
- Substitute `$VAR` with environment variables.
  `$HOME/adb` → `/Users/<user>/adb`
- Resolve `..` directory traversal.
  `../tools/adb` → `<workspace>/../adb`
@buschtoens
Copy link
Copy Markdown
Contributor Author

Great, thank you! I've rebased the PR and fixed all lint issues. 😊


btw I recommend adding prettier for consistent formatting. Your current code style is:

// .prettierrc.json
{
    "trailingComma": "none",
    "tabWidth": 4,
    "singleQuote": false
}

(FWIW I would recommend switching to tabWidth: 2.)

Your eslintrc.json also has a lot of (unintentional?) doubles in. For instance, if @typescript-eslint/no-empty-function is enabled, no-empty-function should be disabled. As the former is a TS-compatible superset of the latter.

As a matter of fact, the @typescript-eslint/recommended config preset intentionally disables all of the eslint:recommended rules, that it itself provides an enhanced version of.

Also, the current rules object needlessly pretty much repeats all of the configuration from the two presets you're already extending from: @typescript-eslint/recommended & @typescript-eslint/recommended-requiring-type-checking.

I recommend deleting all rules, unless you're certain, that the rule is not covered by the two presets, or that you'd like to configfure that rule differently.

This README has more details: https://github.com/typescript-eslint/typescript-eslint/tree/master/packages/eslint-plugin/src/configs#readme

And if you do want to use prettier, you should also extend from the respective eslint-config-prettier, so that all rules that would conflict with prettier's format are disabled.

Depending on your preferences, instead of running prettier as a standanlone tool, you could even invoke prettier as part of eslint via eslint-plugin-prettier.

In case you're not sure how to "correctly" configure eslint and prettier, but would still like to use them, I'd happily submit another PR for that.

@mpotthoff mpotthoff merged commit 521b029 into mpotthoff:master Aug 9, 2021
@mpotthoff
Copy link
Copy Markdown
Owner

Thank you! Do you want me to release a new version with your changes right away or is there anything else you want to change?


(Btw I'm still fond of the 4 spaces indentation - Just my personal preference 😜)

Regarding the eslint configuration - Yeah it's a complete mess. I decided to give tslint-to-eslint-config a try and it created this.... beast of a config. Usually I create these configs from scratch based on some recommendation but I was too lazy this time and also too lazy to properly clean it up afterwards (would properly have ended in me recreating the config from scratch anyway)...

And thank you for your offer but I really don't want to bother you with with boring tasks like cleaning up the linter configuration when I'm just too lazy to do it myself... But I promise that I will do better and clean it up soon! 🙏

@buschtoens buschtoens deleted the patch-1 branch October 4, 2021 12:39
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