Skip to content

Adapt to change in yojson and cmdliner#6

Open
FardaleM wants to merge 4 commits intocyborgize:masterfrom
FardaleM:master
Open

Adapt to change in yojson and cmdliner#6
FardaleM wants to merge 4 commits intocyborgize:masterfrom
FardaleM:master

Conversation

@FardaleM
Copy link

Adapt code to work with yojson 3.* and cmdliner 2.*.

Remove use of deprecated option with atdgen.

@cyberhuman
Copy link
Contributor

Thanks! I think some constraints on package versions must be added.

@FardaleM
Copy link
Author

I am not sure which constraint should be added. I tested it with cmdliner 1.3 and 2.1 both works fine. Same for yojson, I tested it with yojson 2.2.2 and 3.0.0 and both work also. If you want I can put this as lower bound.

(targets config_j.ml config_j.mli)
(deps config.atd)
(action (run atdgen -j -j-std %{deps})))
(action (run atdgen -j %{deps})))
Copy link
Contributor

Choose a reason for hiding this comment

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

is it going to change the behavior if installed in a switch that has atd2 only?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch, I can revert this change or put a lower bound. What would be the best?

Copy link

Choose a reason for hiding this comment

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

just keep the option, it is no-op in newer atdgen

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants