-
Notifications
You must be signed in to change notification settings - Fork 172
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.
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
Begin cleaning up the PPX #534
Conversation
Also, I believe most of this stuff was included in the PPX to make it more obvious how to convert from the Camlp4 syntax. However, these particular features turned out to be not needed by most users. |
May I ask what the rationale is behind this change? I don't mind updating my software, I just prefer having a less noisy version. A common pattern in ppxs to avoid name clashes is to provide both a prefixed and a non-prefixed version. For example, from ppx_deriving:
|
Just to avoid crowding the namespace. I got the impression that the main reason it exists in unprefixed form is that the Lwt Camlp4 syntax had a I wasn't aware of this being a common PPX pattern, if that's so we can remove that commit. It's also the one I'm least convinced about deprecating. |
I checked the reverse dependences for Actually, in all cases except Of course, that only covers code in opam, so I don't really know. |
Here is some quick data point :
|
PS I hope |
I think I agree with @copy on I mostly agree with the rest. |
@Drup, can you say how I find the existence of |
Also I believe @kandu mentioned that it's easier to work on the PPX with |
Okay, I think I will merge this without the commit about deprecating @Drup: @ygrek: Indeed, we won't remove |
Well, Is the conditional code such an issue ? It's fairly well localized and not extremely complicated. |
I don't think either the conditional code nor complex |
2a5ba3f
to
c7ad8b3
Compare
That's totally fair. :) |
This commit deprecates a lot of little-used PPX features, which are probably also somewhat ill-conceived. This includes all PPX options and several bits of syntax:
-no-debug
: There is no point in supporting this (PPX: remove -no-debug option #528). In opam, it is used only insqlexpr
(cc @mfp).-no-sequence
,-no-strict-sequence
: We are removing>>
anyway (Remove >> syntax from the PPX #495), so we should remove these options. They have no users in opam.-log
,-no-log
: We are deprecating Lwt_log, so it's probably worthwhile to remove logging support from the PPX (Delete logging support from the PPX #520). These options have no users in opam.[%lwt ...]
syntax expanding toLwt.catch ...
. This is causing problems ([easy-ish] PPX: replace [%lwt ...] by [%lwt.catch ...] or remove it #527), so we don't want to support it. It is used ineliom
andsqlexpr
(cc @mfp @balat @jrochel @vouillon @vasilisp).[%finally ...]
should be replaced by[%lwt.finally ...]
. The bare[%finally ...]
is used indevkit
,gdb
,gdbprofiler
,sqlexpr
(cc @cyberhuman @ygrek @copy @mfp).Some more notes:
let%lwt ...
structure item becomes a call toLwt_main.run
. It might be impossible to support this on Node.js in a reasonable way. I'm not sure if we want to deprecate it yet, but it's good that it is not documented.-no-sequence
is the most legitimate of the PPX options. Users that want>>
as a definable operator may be using it to suppress Lwt's>>
. I'm not sure if the warning will be too noisy for them.[%lwt let ...]
, just[%lwt ...]
when it would expand toLwt.catch ...
. We should probably disable[%lwt let ...]
and[%lwt ...]
around all other constructs, by checking the location of the%lwt
extension point, as is being done in Ppx: add support for begin%lwt e1; e2; end in ppx extension #525 for;
. This is still being discussed there, so left for future work.Some time after deprecating all this, maybe in
lwt_ppx
2.0.0, we can delete support for all these features from the PPX, making it much simpler than it is now. A side effect of factoring the PPX out intolwt_ppx
is that it can have its own release cycle, so we could do this without a major release of the main packagelwt
.