search

rock-core / vscode-rock

VSCode extension for Rock integration
MIT License
1 stars 1 forks source link

use strict null checks #5

Closed doudou closed 6 years ago

doudou commented 6 years ago

This is a series of change that were triggered while turning on TS strict null checks. It's more than that, and I might have overstepped the mark w.r.t. @g-arjones's initial design. Feel free to tell me if there are things you don't like there ;-)

It's still WIP - i.e. mostly not well-tested.

g-arjones commented 6 years ago

I like it. Other than the commands regression bug and the isInternal() inverted logic, everything else looks good to me. Do you have a milestone? I think this is good enough for a first release/publish.

doudou commented 6 years ago

Do you have a milestone?

I'm trying to get it to work on my machine. I'll then do proper documentation in the README (with screenshots, or a short video) and then I'd say we release 0.1

doudou commented 6 years ago

I also need to release autoproj master before we release this (the output of --tool changed to include the package type)

g-arjones commented 6 years ago

I'm trying to get it to work on my machine.

Hahah..

I'll then do proper documentation in the README (with screenshots, or a short video) and then I'd say we release 0.1

I was considering wiki pages for that.

I also need to release autoproj master before we release this (the output of --tool changed to include the package type)

👍

I also have a few minor things that I think maybe improved in the short term but they are not related so I will open issues so we can discuss them individually.

doudou commented 6 years ago

I've pushed fixes, and added interfacing with envsh (because I was in a case that needed it).

Once thing I wanted to do and couldn't find a way I liked: listen to filesystem events. I wanted to reload the workspace info if installation-manifest changes. The easiest would have been to add it to the Workspace object, but then I would have had to add a dependency on VScode that much deep down. I couldn't find a nice way to do it differently ...

doudou commented 6 years ago

I also have comments, but I'll create discussion threads and publish 0.1.

g-arjones commented 6 years ago

Once thing I wanted to do and couldn't find a way I liked: listen to filesystem events

Related to: #7

The easiest would have been to add it to the Workspace object, but then I would have had to add a dependency on VScode that much deep down. I couldn't find a nice way to do it differently ...

We can't use the file watcher provided by the vscode API because it only watches files that are within vscode's workspace folders (autoproj's root isn't supposed to be added). My idea was to use node's FileSystem module and wrap it in our own FileWatcher class so we can inject it anywhere we want to make it testable.

g-arjones commented 6 years ago

Looks all good! Unless you have further plans, I think you should merge before you publish 0.1.

EDIT: Also, could you add tests for the new methods on autoproj and context modules?

doudou commented 6 years ago

Also, could you add tests for the new methods on autoproj and context modules?

Forgot that ... sorry. I'm trying. Let's just say that I'm becoming that fond of error reporting within typescript / typemoq.

g-arjones commented 6 years ago

I prefer to avoid two people working directly in the same branch. You may be in the middle of a rebase and things can get messy, specially if for some reason you miss the pushed commit notification. What I should have done (and I actually thought I did) was base the PR on master instead...