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.

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

Remove Uchar.dump #7500

Closed
vicuna opened this issue Mar 5, 2017 · 5 comments
Closed

Remove Uchar.dump #7500

vicuna opened this issue Mar 5, 2017 · 5 comments
Assignees
@alainfrisch
Labels
bug stdlib
Milestone
4.05.0

Comments

@vicuna
Copy link

vicuna commented Mar 5, 2017

Original bug ID: 7500
Reporter: @dbuenzli
Assigned to: @alainfrisch
Status: resolved (set by @alainfrisch on 2017-03-07T13:21:23Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.04.0
Target version: 4.05.0 +dev/beta1/beta2/beta3/rc1
Fixed in version: 4.05.0 +dev/beta1/beta2/beta3/rc1
Category: standard library
Monitored by: @dbuenzli

Bug description

There is a bad design error that went unnoticed in the introduction of the Uchar module [0].

The Uchar module depends on Format because of the convenience function Uchar.dump. This prevents using the type in the String, Buffer and Bytes modules where it could naturally occur in the future evolution of the standard library (e.g. I was preparing this [1] when I noticed this).

While a few workarounds could be provided (e.g. via a definition in Pervasives) it seems the best is to simply remove the function and provide it in Format as Format.pp_print_dump_uchar.

Since Uchar.dump is mainly to be used as a debugging function I expect breakage to be minimal [2]. If there is consensus this should be done, I will do it asap.

[0] #80
[1] dbuenzli@b62b32d
[2] https://github.com/search?l=OCaml&q=%22Uchar.dump%22&ref=searchresults&type=Code&utf8=%E2%9C%93
@vicuna
Copy link
Author

vicuna commented Mar 5, 2017

Comment author: @gasche

The rationale looks very convincing to me, and if we decide to make the change, I would propose to do it as soon as possible: in 4.05. The more we wait, the more people are likely to use the function.

@vicuna
Copy link
Author

vicuna commented Mar 5, 2017

Comment author: @dbuenzli

#1081

@vicuna
Copy link
Author

vicuna commented Mar 5, 2017

Comment author: @dbuenzli

So this affects Uunf, Uuseg and Uutf but I have patches and I'm ready to release if GPR1081 gets merged. FWIW I took me time to accept the idea but I really think it's the best solution. Sorry for the mess up.

@vicuna
Copy link
Author

vicuna commented Mar 5, 2017

Comment author: @dra27

I've just commented on the GPR, but I don't think a breaking change like that is likely for 4.05 at this stage? However, perhaps deprecating Uchar.dump in 4.05 could be then remove for 4.06?

@vicuna
Copy link
Author

vicuna commented Mar 5, 2017

Comment author: @dbuenzli

Damien, ping me if you prefer the deprecation route. I'll prepare a GPR.

@vicuna vicuna closed this as completed Mar 7, 2017
@vicuna vicuna added the stdlib label Mar 14, 2019
@vicuna vicuna added this to the 4.05.0 milestone Mar 14, 2019
@vicuna vicuna added the bug label Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@alainfrisch alainfrisch

Labels
bug stdlib
Projects
None yet
Milestone
4.05.0
Development

No branches or pull requests
2 participants
@alainfrisch @vicuna