Completions based on your path #17

mads-hartmann · 2018-04-01T17:30:42Z

No description provided.

mads-hartmann · 2018-04-01T17:31:41Z

@skovhus I'd love if you could give this a review. I realise it's a bit messy, sorry about that :)

skovhus

Good stuff! If you have time it would be super nice to have one PR with the change to move to a class based approach. And the motivation. : )

skovhus · 2018-04-01T17:42:17Z

server/src/__tests__/executables.test.ts

+    return expect(result).resolves.toBeTruthy()
+  })
+
+  it.skip('It only considers files that have the executable bit set', () => {


There is a skip here.

Btw, you should remove the “It” from the message.

@skovhus The skip is there because the test fails currently as it doesn't look at the executable bit yet 😅

skovhus · 2018-04-01T17:46:35Z

server/src/__tests__/executables.test.ts

+
+  it('It only considers executable directly on the PATH', () => {
+    expect.assertions(1)
+    const result = executables.then(ex =>


Using await in these tests might make it easier. Then you don’t need the expect.assertions. : )

skovhus · 2018-04-01T17:47:19Z

server/src/analyser.ts

+   * Initialize the Analyzer based on a connection to the client and an optional
+   * root path.
+   *
+   * If the rootPath is provided it will iniailize all *.sh files it can find


Spelling: iniailize

skovhus · 2018-04-01T17:47:54Z

server/src/analyser.ts

- *
- * Returns all, if any, syntax errors that occurred while parsing the file.
- *
+ * The Analyzer uses the Abstract Syntax Trees (ASTs) that are provdied by


Spelling: provdied

skovhus · 2018-04-01T17:49:11Z

server/src/analyser.ts

-}
+  // Global mapping from tree-sitter node type to vscode SymbolKind
+  private kinds: Kinds = {
+    environment_variable_assignment: LSP.SymbolKind.Variable,


Any reason why we change to underscore here?

mads-hartmann · 2018-04-01T18:43:30Z

@skovhus I moved to using classes as I didn't like having state in the modules as top-level consts. With a class based approach it's clear what the state is and where it lives :) Are you against using classes? If so why? 😊

skovhus · 2018-04-01T19:31:57Z

server/src/__tests__/executables.test.ts

  path.resolve(__dirname, '..', '..', '..', 'testing', 'executables'),
 )

 describe('list', () => {
-  it('It finds executables on the PATH', () => {
+  it('finds executables on the PATH', async () => {
    expect.assertions(1)


Not that you are using await you don't need:

expect.assertions(1)

and returning the last expect

@mads-hartmann this was the reason why I suggested using await here, so we could get rid of these extra things you need to remember to do. : )

skovhus · 2018-04-01T19:43:28Z

With a class based approach it's clear what the state is and where it lives :) Are you against using classes? If so why?

So if you need state (which you do), then I usually go for small files with top-level state variable(s). The singleton module pattern makes this possible... I'm just very used to this pattern.

If using classes makes this more clear then I'm all for it. 👍But interesting if I should start using this more... 🤔

skovhus · 2018-04-01T19:49:01Z

server/src/analyser.ts

-        SymbolInformation.create(name, kinds[n.type], range(named), uri),
-      )
-      declarations[uri][name] = namedDeclarations
+export class Analyzer {


I think we should an export default class Analyzer here, so import would be import Analyzer from ....

skovhus · 2018-04-01T19:51:20Z

server/src/executables.ts

+/**
+ * Provides information based on the programs on your PATH
+ */
+export class Executables {


I suggest export default

skovhus · 2018-04-01T19:52:51Z

server/src/executables.ts

+ * Find all the executables on the given path.
+ * Only returns direct children, or the path itself if it's an executable.
+ */
+function _executables(path: string): Promise<string[]> {


We would call this findExecutablesInPath, then we can almost get rid of the code comment.

skovhus · 2018-04-01T19:57:06Z

server/src/util/sh.ts

+import * as ChildProcess from 'child_process'
+
+/**
+ * Execute the following sh program.


Nitpicking: I prefer not having code comments like this and make the function name more descriptive: execShellScript(body: string)

We could use https://github.com/sindresorhus/execa for all child process handling. It has a few advantages but also introduces more dependencies. Pros cons.

@skovhus Let's switch to execa if we find bugs in this code :)

skovhus · 2018-04-01T20:00:34Z

server/src/index.ts

+      }))
+  })
+
+  // Listen on the connection


Nitpicking: superfluous comment... In general, I really try to get rid of most code comments as:

they quickly become outdated

the code can often explain itself (or at least it should)

when code needs a comment (and sometimes it really does), the comment now becomes visible (as less important comments are gone)

skovhus · 2018-04-01T20:00:57Z

server/src/server.ts

+ * The BashServer glues together the separate components to implement
+ * the various parts of the Language Server Protocol.
+ */
+export class BashServer {


I suggest export default

skovhus · 2018-04-01T20:04:20Z

server/src/analyser.ts

-    }
+  // Global mapping from uri to the contents of the file at that uri.
+  // We need this to find the word at a given point etc.
+  private texts: Texts = {}


We would rename all of these, so the comment would become superfluous.

Example: uriToFileContent, uriToTreeSitterDocument. I would prefer that. : )

It uses the MAN pages for the executable to provide extra information on hover and in the completion selection menu. I also ended up refactoring most of the code so this is a bit of a messy commit. Sorry.

mads-hartmann self-assigned this Apr 1, 2018

mads-hartmann mentioned this pull request Apr 1, 2018

WIP: A bit of refactoring #16

Closed

skovhus reviewed Apr 1, 2018

View reviewed changes

mads-hartmann added 7 commits April 7, 2018 22:25

Provide completions based on the executables on your PATH

f6dcf2d

It uses the MAN pages for the executable to provide extra information on hover and in the completion selection menu. I also ended up refactoring most of the code so this is a bit of a messy commit. Sorry.

Add a couple of tests for Executables

fd66cad

Switch to using async/await

f83f7b5

Fix typos in comments

be95807

Add note about snake-case usage

a5d059f

Code review: Use default exports

8ed0d11

Code review: Better names, less comments

be1d796

mads-hartmann force-pushed the completions-based-on-your-PATH branch from 9dae994 to be1d796 Compare April 7, 2018 20:25

skovhus added 2 commits April 7, 2018 22:44

Add getWordAtPoint helper function

075c521

Make symbol completions unique

3dfff09

skovhus merged commit b8d9f07 into master Apr 7, 2018

skovhus deleted the completions-based-on-your-PATH branch April 7, 2018 21:34

vonschutter mentioned this pull request Feb 27, 2024

Bash symbol list is incomplete #1108

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Completions based on your path #17

Completions based on your path #17

mads-hartmann commented Apr 1, 2018

mads-hartmann commented Apr 1, 2018

skovhus left a comment

skovhus Apr 1, 2018

mads-hartmann Apr 1, 2018

skovhus Apr 1, 2018

skovhus Apr 1, 2018

skovhus Apr 1, 2018

skovhus Apr 1, 2018

mads-hartmann commented Apr 1, 2018

skovhus Apr 1, 2018

skovhus Apr 1, 2018

skovhus commented Apr 1, 2018 •

edited

Loading

skovhus Apr 1, 2018

skovhus Apr 1, 2018 •

edited

Loading

skovhus Apr 1, 2018

skovhus Apr 1, 2018

skovhus Apr 1, 2018

mads-hartmann Apr 7, 2018

skovhus Apr 1, 2018 •

edited

Loading

skovhus Apr 1, 2018 •

edited

Loading

skovhus Apr 1, 2018

Completions based on your path #17

Completions based on your path #17

Conversation

mads-hartmann commented Apr 1, 2018

mads-hartmann commented Apr 1, 2018

skovhus left a 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

mads-hartmann commented Apr 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skovhus commented Apr 1, 2018 • edited Loading

Choose a reason for hiding this comment

skovhus Apr 1, 2018 • edited Loading

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

skovhus Apr 1, 2018 • edited Loading

Choose a reason for hiding this comment

skovhus Apr 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skovhus commented Apr 1, 2018 •

edited

Loading

skovhus Apr 1, 2018 •

edited

Loading

skovhus Apr 1, 2018 •

edited

Loading

skovhus Apr 1, 2018 •

edited

Loading