Store configuration options for multiple kernels #474
Conversation
|
|
|
I found some issue after initialization parameters-mixer plugin, although tests passed |
|
Solved |
|
Solved #467 also on this PR |
I've added kernel name as third param. |
src/debugger-configuration.ts
Outdated
| } | ||
|
|
||
| private _hashMethod: (code: string) => string; | ||
| private _tmpFileAssociatedWithKernel = new Map<string, [string, string]>(); |
There was a problem hiding this comment.
Maybe there could be an interface instead of [string, string]?
So it's easier to follow what they refer to.
src/debugger-configuration.ts
Outdated
| setHashParameters(method: string, seed: number, kernelName: string): void; | ||
| setTmpFileParameters( | ||
| prefix: string, | ||
| suffix: string, | ||
| kernelName: string | ||
| ): void; |
There was a problem hiding this comment.
We probably want to pass an object with the method, seed and kernelName properties instead of multiple arguments to the function, so it's easier to extend later if needed.
|
I agree with the latter point. The former point also seems like a good
idea. I'll be pushing an update to this.
…On Wed, Jul 8, 2020 at 4:10 PM Jeremy Tuloup ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/debugger-configuration.ts
<#474 (comment)>:
> + setHashParameters(method: string, seed: number, kernelName: string): void;
+ setTmpFileParameters(
+ prefix: string,
+ suffix: string,
+ kernelName: string
+ ): void;
We probably want to pass an object with the method, seed and kernelName
properties instead of multiple arguments to the function, so it's easier to
extend later if needed.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#474 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABG6KPI5OZ3WSRQY5Q7KQLR2SD7VANCNFSM4OARNFTQ>
.
|
|
We should now be able to rebase the PR to fix the tests. I'll give the PR a quick test locally too. |
src/editor-finder.ts
Outdated
| focus: boolean, | ||
| kernelName: string |
There was a problem hiding this comment.
Similar comment as above. These findIn* methods used to have 2 arguments only, but now that they are growing we might want to refactor that into an object too.
src/debugger-configuration.ts
Outdated
| import { murmur2 } from 'murmurhash-js'; | ||
|
|
||
| /** | ||
| * A class to hash code. |
There was a problem hiding this comment.
How about something more specific?
| * A class to hash code. | |
| * A class that holds debugger configuration for a kernel. |
There was a problem hiding this comment.
It makes sense
|
Just tried locally and it looks good, thanks @KrzysztofSikoraCodete 👍 Having the debug config (hash functions and parameters) specific to a kernel indeed makes sense.
@afshin looks like we should be able to merge this PR in once rebased and updated, so it can be included in the next release. |
|
Nice to hear that. I'll open new PR after merge this one, then i'll improve about what in the above remarks. |
I thought we could do it in the same PR here? |
Right, so i'll do here |
1de49e7 to
1681f75
Compare
ad5c76a to
be605a0
Compare
e1909f9 to
d5fff95
Compare
|
Excellent idea. Thanks @afshin for refactor and cleaning |
…lgorithms and remove superfluous dispose method
a5b33db to
b618d35
Compare
15e19f5 to
8937201
Compare
Related with issue:
#471
Summary:
Added editor finder to console and file handler