MikeZ's Answers #17
MikeZ's Answers #17
Conversation
|
This pull request introduces 12 alerts and fixes 3 when merging 829d563 into e1e5f41 - view on LGTM.com new alerts:
fixed alerts:
|
ephox-mogran
left a comment
There was a problem hiding this comment.
Looks good.
How did you find it?
| <!-- | ||
| <script | ||
| src="https://cdn.tiny.cloud/1/7wn8emqewsyc737d25kijfc0xlautpx1jjorbvsdqqu38w85/tinymce/6/tinymce.min.js" | ||
| referrerpolicy="origin" |
There was a problem hiding this comment.
So, can you remember what referrerpoilcy is for :) ?
There was a problem hiding this comment.
From what I can recall:
referrerpolicy specifies the header of requests to the server. When it is set to origin, it would be set to the base url, which is beneficial for the customer because it encourages all requests to cache to the same place and retrieve from that cache too, speeding things up. It's also beneficial for us as it would reduce the number of editor loads.
| export const width = (b: Boundz): number => b.x2 - b.x1; | ||
|
|
||
| // DONE implement height function | ||
| export const height = (b: Boundz): number => b.y2 - b.y1; |
There was a problem hiding this comment.
What about if y1 > y2 and the bounds didn't have to be top-left to bottom-right. How would you model height then?
| { name: 'frog2', ribbits: false, age: 4 }, | ||
| { name: 'loudfrog', ribbits: true, age: 1 }, | ||
| { name: 'quietfrog', ribbits: false, age: 10 }, | ||
| { name: "frog1", ribbits: true, age: 3 }, |
There was a problem hiding this comment.
Yeah good point, I'm trying to configure my formatter to use the linting rules in .editorConfig and .eslintrc.json in the main tinymce repo. Do you use a JS/TS formatter like Prettier?
|
|
||
| // DONE: Write a function that converts an A[] to an Optional<A>. If the array has more than one element, only consider the first element. | ||
| export const arrayToOptional = <A>(arr: A[]): Optional<A> => | ||
| arr.length === 0 ? Optional.none() : Optional.some(arr[0]); |
There was a problem hiding this comment.
Nice. It's not uncommon for people to put Optional.from here for arr[0], but that's incorrect. What you have is correct. The some value can be undefined or null in strange cases.
| */ | ||
|
|
||
| const getOrElse1Constant = <A>(oa: Optional<A>, other: A): A => | ||
| oa.fold(constant(other), Fun.identity); |
There was a problem hiding this comment.
In what situation would you avoid using constant do you think?
There was a problem hiding this comment.
Within the context of the getOrElse1 function, I can't think of a situation where constant needs to be avoided.
| it("toPositiveInteger", () => { | ||
| // DONE: write a few test cases | ||
| assert.isTrue(Ex.toPositiveInteger(5).equals(Optional.some(5))); | ||
| assert.isTrue(Ex.toPositiveInteger(0).isNone()); |
There was a problem hiding this comment.
Is 0 not a positive integer? Is there a definitive answer on this somewhere?
There was a problem hiding this comment.
I've always considered 0 as neither negative nor positive. Some literature specify "non-negative" integers to specify 0 and positive integers.
| assert.deepEqual(Ex.optionalToArray(Optional.none()), []); | ||
| }); | ||
|
|
||
| it("arrayToOptional", () => { |
There was a problem hiding this comment.
Testing arrays of null and undefined would be good as well.
There was a problem hiding this comment.
Good point,
for optionalToArray:
assert.deepEqual(Ex.optionalToArray(Optional.some(undefined)), [undefined]);
assert.deepEqual(Ex.optionalToArray(Optional.some(null)), [null]);
for arrayToOptional:
assert.isTrue(Ex.arrayToOptional([undefined, undefined, undefined]).equals(
Optional.some(undefined)
)
);
assert.isTrue(
Ex.arrayToOptional([null, null, null]).equals(Optional.some(null))
);
No description provided.