Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add flowconfig parser js_of_ocaml package #7698

Closed
wants to merge 3 commits into from
Closed

Add flowconfig parser js_of_ocaml package #7698

wants to merge 3 commits into from

Conversation

goodmind
Copy link
Contributor

@goodmind goodmind commented May 4, 2019

image

mroch
mroch reviewed May 6, 2019
View reviewed changes
Copy link
Contributor

@mroch mroch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool! i don't know if it's your end goal or not, but this could make it possible to test flowconfig in Try Flow. (aside: if your goal is to allow config changes in Try Flow, flowconfig isn't necessary, we can discuss further)

js/str.js
@@ -1,14 +1,217 @@
// Partially from
// https://github.com/ejgallego/jscoq/blob/2718f9caf31398704c2d84ff089e3f6f0321eada/coq-js/js_stub/str.js
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is AGPL-3+, and the part based on strstubs.c is LGPL2 + static linking exception. we will have to review license compatibility...

without looking into how hard it'd be first, i'd be more inclined to remove the uses of Str instead. might even make sense to use a real parser (we already depend on sedlex and ocamllex) but that's a bigger project.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll remove it for now, I think LGPL2 is compatible with MIT, not sure about AGPL3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/ejgallego/jscoq/issues/97

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Str also would be useful for flow.js overall, not just flowconfig parser

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Js_of_ocaml (3.6) now include all full support for Str

src/commands/config/js/stdlib.js
@@ -0,0 +1,21 @@
// Stdlib overrides until js_of_ocaml fixes it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there an upstream github issue we can reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ocsigen/js_of_ocaml#791

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was fixed, but I know that Flow uses old js_of_ocaml

@goodmind
Copy link
Contributor Author

goodmind commented May 6, 2019

I'm not sure about specific goal, I heard that babel needed flowconfig parsing for all option, also this can incorporate flow init but accept js objects instead of CLI

@goodmind
Copy link
Contributor Author

goodmind commented Jun 4, 2019

@mroch I think LICENSE was updated

Files: coq-js/js_stub/str.js
License: LGPL2.1
Comment: Derived from OCaml (https://github.com/ocaml/ocaml).

Is this still doesn't work with MIT?

@goodmind
Copy link
Contributor Author

goodmind commented Jun 4, 2019

My goal is to allow any js tool to write or read flowconfig, related #7771

@goodmind
Copy link
Contributor Author

/cc @mroch does LICENSE still bad one? It is LGPL2.1 now

This was referenced Jul 21, 2019
Closed
Open
@goodmind goodmind added the Stalled Issues and PRs that are stalled. label Jul 27, 2019
hhugo
hhugo reviewed May 2, 2020
View reviewed changes
src/commands/config/js/stdlib.js
case 120: case 88: base = 16; i += 2; break;
case 111: case 79: base = 8; i += 2; break;
case 98: case 66: base = 2; i += 2; break;
case 117: case 85: sign = 0; i += 2; break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sign = 0 is not correct.

@SamChou19815
Copy link
Contributor

Closing since there are license issues, and we already have support for flowconfig in try-flow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mroch mroch mroch left review comments

@hhugo hhugo hhugo left review comments

Assignees
No one assigned
Labels
CLA Signed flowconfig Stalled Issues and PRs that are stalled.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@goodmind @SamChou19815 @mroch @hhugo @facebook-github-bot