content script now doesn't rewrite code as refmt errors #92
Conversation
improved Refmt2: - eliminated Refmt2.refmtJS - Refmt2.refmt now uses bs-result protocol improved: - made refmt message serialization format more friendly: - using string for tag instead of integer - using jsConverter for payload record limitations: - Refmt2.refmt really needs a more complex result type to report degree of success or failure - code on page should contain the error message somewhere, maybe in a tooltip
| @@ -54,7 +54,9 @@ let getBlacklist = () => [ | |||
| "mwhittaker.github.io/distributed-systems-ocaml/code_MorePipes.html" /* #49 */ | |||
| ]; | |||
|
|
|||
| let getWhitelist = () => []; | |||
| let getWhitelist = () => [ | |||
| "codebad.com/~hdon/reason-tools-test.html" | |||
There was a problem hiding this comment.
Thanks for adding this test!
src/refmt/refmt2.re
Outdated
| | a => (UnknownLang, UnknownLang, Obj.magic(a)) | ||
| | a => { | ||
| Js.log("hdon sez: refmt2 is error"); | ||
| Result.Error(a |> Printexc.to_string) |
There was a problem hiding this comment.
This doesn't seem to be giving very useful responses. Everything gets printed as Failure(_) when I try. I think this can be fixed in a followup.
There was a problem hiding this comment.
Yeah it's definitely got some limitations. I was hoping to get some feedback about it.
There was a problem hiding this comment.
Yeah I think Printexc.print gives more control over the actual output here so we can make sure things look nice.
| @@ -38,6 +38,7 @@ | |||
| }, | |||
| "devDependencies": { | |||
| "bs-platform": "^2.2.1", | |||
| "bs-result": "^1.1.0", | |||
There was a problem hiding this comment.
Thanks! Much more reliable.
src/extension/common/protocol.re
Outdated
| type payload = { | ||
| outText: string, | ||
| inLang: language, | ||
| outLang: language | ||
| }; | ||
| type response = result(payload, string); | ||
| type response = Result.t(payload, string); |
There was a problem hiding this comment.
When I compile this code I get an error here. It looks like it should be Result.result(string, payload);
I'm guessing it is a version issue for bs-result?
| @@ -1,22 +1,19 @@ | |||
| open Core; | |||
|
|
|||
| module Refmt = { | |||
| let refmt = Refmt2.refmtJS; | |||
| let refmt = Refmt2.refmt; | |||
| let parse = | |||
There was a problem hiding this comment.
Why not use Result.map?
let parse = Result.map(((inlang, outLang, outText)) =>
Protocol.Refmt.{ outText, inLang, outLang };
| type result('a, 'e) = | ||
| | Ok('a) | ||
| | Error('e); | ||
| open Result; |
There was a problem hiding this comment.
No need for the open since all the Result namespace is used.
There was a problem hiding this comment.
Yeah IDK why that's there :3
|
Any chance we could get a bamp-release to AMO, so we can close #91? I've currently gotta keep the extension disabled, and manually re-enable it every time I have a use for it! |
Feedback would be greatly appreciated!
Edit: The main reason for this PR was to address the extension's behavior when a syntax error occurs. In
masterbranch, the behavior is to destroy the original source code on your page by replacing it with an error message. This PR fixes that. A future PR could improve it further by putting the error message some place the user can find it.improved Refmt2:
protocol improved:
limitations:
of success or failure
tooltip