Completions based on your path #17

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

Jump to bottom

Merged

skovhus merged 9 commits into master from completions-based-on-your-PATH

Apr 7, 2018

Merged

Completions based on your path #17

skovhus merged 9 commits into master from completions-based-on-your-PATH

Apr 7, 2018

Conversation

mads-hartmann

Copy link

Collaborator

@mads-hartmann mads-hartmann commented Apr 1, 2018

No description provided.

@mads-hartmann mads-hartmann self-assigned this

Apr 1, 2018

@mads-hartmann

Copy link

Collaborator Author

mads-hartmann commented Apr 1, 2018

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

@mads-hartmann mads-hartmann mentioned this pull request

Apr 1, 2018

WIP: A bit of refactoring #16

Closed

skovhus

skovhus reviewed

Apr 1, 2018

View reviewed changes

Copy link

Collaborator

@skovhus skovhus left a comment

Choose a reason for hiding this comment

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

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. : )

server/src/__tests__/executables.test.ts Outdated

return expect(result).resolves.toBeTruthy()

})

it.skip('It only considers files that have the executable bit set', () => {

Copy link

Collaborator

Choose a reason for hiding this comment

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

There is a skip here.

Btw, you should remove the "It" from the message.

Copy link

Collaborator Author

@mads-hartmann mads-hartmann Apr 1, 2018

Choose a reason for hiding this comment

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

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

server/src/__tests__/executables.test.ts Outdated

it('It only considers executable directly on the PATH', () => {

expect.assertions(1)

const result = executables.then(ex =>

Copy link

Collaborator

Choose a reason for hiding this comment

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

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

server/src/analyser.ts Outdated

* 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

Copy link

Collaborator

Choose a reason for hiding this comment

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

Spelling: iniailize

server/src/analyser.ts Outdated

* Returns all, if any, syntax errors that occurred while parsing the file.

* The Analyzer uses the Abstract Syntax Trees (ASTs) that are provdied by

Copy link

Collaborator

Choose a reason for hiding this comment

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

Spelling: provdied

server/src/analyser.ts Outdated

}

// Global mapping from tree-sitter node type to vscode SymbolKind

private kinds: Kinds = {

environment_variable_assignment: LSP.SymbolKind.Variable,

Copy link

Collaborator

Choose a reason for hiding this comment

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

Any reason why we change to underscore here?

@mads-hartmann

Copy link

Collaborator Author

mads-hartmann commented Apr 1, 2018

@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

skovhus reviewed

Apr 1, 2018

View reviewed changes

server/src/__tests__/executables.test.ts Outdated

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

)

describe('list', () => {

it('It finds executables on the PATH', () => {

it('finds executables on the PATH', async () => {

expect.assertions(1)

Copy link

Collaborator

@skovhus skovhus Apr 1, 2018

Choose a reason for hiding this comment

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

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

expect.assertions(1)
and returning the last expect

Copy link

Collaborator

@skovhus skovhus Apr 1, 2018

Choose a reason for hiding this comment

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

@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

Copy link

Collaborator

skovhus commented Apr 1, 2018 •

edited

Loading

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

skovhus reviewed

Apr 1, 2018

View reviewed changes

server/src/analyser.ts Outdated

SymbolInformation.create(name, kinds[n.type], range(named), uri),

)

declarations[uri][name] = namedDeclarations

export class Analyzer {

Copy link

Collaborator

@skovhus skovhus Apr 1, 2018

Choose a reason for hiding this comment

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

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

server/src/executables.ts Outdated

/**

* Provides information based on the programs on your PATH

export class Executables {

Copy link

Collaborator

@skovhus skovhus Apr 1, 2018 •

edited

Loading

Choose a reason for hiding this comment

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

I suggest export default

server/src/executables.ts Outdated

* 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[]> {

Copy link

Collaborator

@skovhus skovhus Apr 1, 2018

Choose a reason for hiding this comment

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

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

server/src/util/sh.ts

import * as ChildProcess from 'child_process'

/**

* Execute the following sh program.

Copy link

Collaborator

@skovhus skovhus Apr 1, 2018

Choose a reason for hiding this comment

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

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

Copy link

Collaborator

@skovhus skovhus Apr 1, 2018

Choose a reason for hiding this comment

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

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.

Copy link

Collaborator Author

@mads-hartmann mads-hartmann Apr 7, 2018

Choose a reason for hiding this comment

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

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

server/src/index.ts Outdated

}))

})

// Listen on the connection

Copy link

Collaborator

@skovhus skovhus Apr 1, 2018 •

edited

Loading

Choose a reason for hiding this comment

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

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)

server/src/server.ts Outdated

* The BashServer glues together the separate components to implement

* the various parts of the Language Server Protocol.

export class BashServer {

Copy link

Collaborator

@skovhus skovhus Apr 1, 2018 •

edited

Loading

Choose a reason for hiding this comment

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

I suggest export default

server/src/analyser.ts Outdated

}

// 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 = {}

Copy link

Collaborator

@skovhus skovhus Apr 1, 2018

Choose a reason for hiding this comment

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

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

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

mads-hartmann added 7 commits

April 7, 2018 22:25

@mads-hartmann


 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.

@mads-hartmann


 Add a couple of tests for Executables

fd66cad

@mads-hartmann


 Switch to using async/await

f83f7b5

@mads-hartmann


 Fix typos in comments

be95807

@mads-hartmann


 Add note about snake-case usage

a5d059f

@mads-hartmann


 Code review: Use default exports

8ed0d11

@mads-hartmann


 Code review: Better names, less comments

be1d796

@mads-hartmann 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

@skovhus


 Add getWordAtPoint helper function

075c521

@skovhus


 Make symbol completions unique

3dfff09

@skovhus skovhus merged commit b8d9f07 into master

Apr 7, 2018

@skovhus skovhus deleted the completions-based-on-your-PATH branch

April 7, 2018 21:34

@vonschutter vonschutter mentioned this pull request

Feb 27, 2024

Bash symbol list is incomplete #1108

Open

Labels

None yet

2 participants

@mads-hartmann @skovhus

Completions based on your path #17

Completions based on your path #17

Uh oh!

Conversation

@mads-hartmann mads-hartmann commented Apr 1, 2018

Uh oh!

mads-hartmann commented Apr 1, 2018

Uh oh!

@skovhus skovhus left a comment

Choose a reason for hiding this comment

Uh oh!

@skovhus skovhus Apr 1, 2018

Choose a reason for hiding this comment

Uh oh!

@mads-hartmann mads-hartmann Apr 1, 2018

Choose a reason for hiding this comment

Uh oh!

@skovhus skovhus Apr 1, 2018

Choose a reason for hiding this comment

Uh oh!

@skovhus skovhus Apr 1, 2018

Choose a reason for hiding this comment

Uh oh!

@skovhus skovhus Apr 1, 2018

Choose a reason for hiding this comment

Uh oh!

@skovhus skovhus Apr 1, 2018

Choose a reason for hiding this comment

Uh oh!

mads-hartmann commented Apr 1, 2018

Uh oh!

@skovhus skovhus Apr 1, 2018

Choose a reason for hiding this comment

Uh oh!

@skovhus skovhus Apr 1, 2018

Choose a reason for hiding this comment

Uh oh!

skovhus commented Apr 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

@skovhus skovhus Apr 1, 2018

Choose a reason for hiding this comment

Uh oh!

@skovhus skovhus Apr 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@skovhus skovhus Apr 1, 2018

Choose a reason for hiding this comment

Uh oh!

@skovhus skovhus Apr 1, 2018

Choose a reason for hiding this comment

Uh oh!

@skovhus skovhus Apr 1, 2018

Choose a reason for hiding this comment

Uh oh!

@mads-hartmann mads-hartmann Apr 7, 2018

Choose a reason for hiding this comment

Uh oh!

@skovhus skovhus Apr 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@skovhus skovhus Apr 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@skovhus skovhus Apr 1, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

skovhus commented Apr 1, 2018 •

edited

Loading

@skovhus skovhus Apr 1, 2018 •

edited

Loading

@skovhus skovhus Apr 1, 2018 •

edited

Loading

@skovhus skovhus Apr 1, 2018 •

edited

Loading