Create platform directory structure #9439
Conversation
.vscode/launch.json
Outdated
| @@ -140,7 +140,7 @@ | |||
| ], | |||
| "env": { | |||
| "VSC_JUPYTER_FORCE_LOGGING": "1", | |||
| "VSC_JUPYTER_CI_TEST_GREP": "", // Leave as `VSCode Notebook` to run only Notebook tests. | |||
There was a problem hiding this comment.
Whoops. Didn't mean to change this
Codecov Report
@@ Coverage Diff @@
## main #9439 +/- ##
======================================
- Coverage 74% 73% -1%
======================================
Files 219 228 +9
Lines 10939 10500 -439
Branches 1571 1538 -33
======================================
- Hits 8127 7700 -427
+ Misses 2185 2174 -11
+ Partials 627 626 -1
|
src/datascience-ui/common/index.ts
Outdated
| @@ -108,7 +109,7 @@ export function formatStreamText(str: string): string { | |||
| export function appendLineFeed(arr: string[], modifier?: (s: string) => string) { | |||
| return arr.map((s: string, i: number) => { | |||
| const out = modifier ? modifier(s) : s; | |||
| return i === arr.length - 1 ? `${out}` : `${out}\n`; | |||
| return i === arr.length - 1 ? `${out}` : `${out}${os.EOL}`; | |||
There was a problem hiding this comment.
Not sure this is correct. I remember Jupyter used just \n (line feeds).
E.g. if we have some multi-line code in a non-python kernel, then it could crash (I know R kernel doesn't support CRLF & we had to ensure we didn't use that else the execution would just crash)
There was a problem hiding this comment.
I'll make it only used for export.
There was a problem hiding this comment.
i think we can't change even for export, sas we'd end up with crlf in ipynb, and this isn't going to work when anyone execute these cells for R or the like.
out of curiosity, what prompted the change?
There was a problem hiding this comment.
Export tests were failing and the output made them look the same.
There was a problem hiding this comment.
ipynb already remove any \r\n in them. However export generates python files which assumes the same EOL as the OS. The tests actually verify this is the case.
There was a problem hiding this comment.
Oh this is export ipynb to py. i thought it was the other way around.
src/platform/ioc/serviceManager.ts
Outdated
| @@ -8,7 +8,7 @@ type identifier<T> = string | symbol | Newable<T> | Abstract<T>; | |||
|
|
|||
| @injectable() | |||
| export class ServiceManager implements IServiceManager { | |||
| constructor(private container: Container) {} | |||
| constructor(public readonly container: Container) {} | |||
There was a problem hiding this comment.
Can we create a property with a simple getter that has a decorator to ensure this gets accessed only during tests.
I.e. use the @testOnlyMethod decorator that Ian added.
There was a problem hiding this comment.
Not required, i think its a good practice
There was a problem hiding this comment.
Good idea. Didn't realize we had a testOnlyDecorator.
There was a problem hiding this comment.
Yeah, I added it for the WebViewHost I think if you want to see it used. Do wonder about the best was to sometimes use these decorators, like I'm not sure how you would be expected to know that we added one? Util stuff just kinda hide out of the way sometimes.
There was a problem hiding this comment.
FYI, the decorator doesn't work on a getter. Throws an error. I had to change this to a function getContainer
|
@rchiodo Nice work! I 100% say would not change this now (or ever maybe), but just something that I like is if the ordering lines up for the functions. For example there is registerPlatformTypes and processRegisterTypes. I would slightly prefer having them both follow the same ordering like registerPlatformTypes and registerProcessTypes. Or switch to platformRegisterTypes and processRegisterTypes. Just makes them easier to find searching if they follow the same format. |
Fixes #8981
Did a bunch of stuff: