Navigation Menu

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

ICU-13685 c: add @preview (retry) #8

Merged
merged 2 commits into from Aug 7, 2018
Merged

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Jul 13, 2018

ICU-13685

re-open from #7

Tech Preview should not be 'do not use', but @internal in Doxygen comes with an ominous warning.

Add @preview as a doxygen command such that:

@preview ICU 4.4

gives:

atpreview

@srl295 srl295 self-assigned this Jul 13, 2018
@srl295 srl295 requested review from sffc and markusicu July 13, 2018 15:15
@jefgen jefgen self-requested a review July 13, 2018 19:00
Copy link
Member

@jefgen jefgen left a comment

Choose a reason for hiding this comment

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

FWIW, LGTM.

@srl295 srl295 changed the title c: add @preview (retry) ICU-13685 c: add @preview (retry) Jul 18, 2018
@jefgen jefgen assigned jefgen and unassigned srl295 Aug 1, 2018
jefgen
jefgen previously approved these changes Aug 1, 2018
@sffc
Copy link
Member

sffc commented Aug 1, 2018

How does this work with the U_HIDE_INTERNAL_API flags? Do we apply that to both @internal and @preview, or do we make a new flag like U_HIDE_PREVIEW_API?

@jefgen
Copy link
Member

jefgen commented Aug 1, 2018

I think most (all?) of these are already behind #ifndef U_HIDE_INTERNAL_API conditions, but I wonder if a
#ifndef U_HIDE_PREVIEW_API might be more clear?

@srl295
Copy link
Member Author

srl295 commented Aug 2, 2018

@jefgen these should already be behind hide internal API. If we want a hide preview API we should probably do that separately.

@srl295
Copy link
Member Author

srl295 commented Aug 2, 2018

okay, conflicts with #29 now

@srl295
Copy link
Member Author

srl295 commented Aug 2, 2018

@sffc actually, i would kind of be inclined to change preview to be behind U_DRAFT and U_HIDE_DRAFT_API — because really, it is more of a draft API than internal.

but i could see the argument for a separate U_PREVIEW and U_HIDE_PREVIEW_API (which defaults to being controlled by U_HIDE_DRAFT_API) - what do you think?

@srl295 srl295 removed the request for review from aheninger August 2, 2018 15:46
@sffc
Copy link
Member

sffc commented Aug 2, 2018

The real difference between @draft and @preview is that the latter does not need to go through as formal of a review process. I've put APIs in both buckets, and I treat changes to @draft much more carefully than @preview. For example, when I changed the name "Rounder" to "Precision" in 62.1, I added a backwards-compatibility API to allow people to transition to the new name; if the API had been @preview instead of @draft, I would have probably wouldn't have gone through that extra step.

In other words, in terms of stability, I see:

  • @stable is the most stable; we do our best to guarantee that users' code won't break if they use this API.
  • @draft is mostly stable but still subject to some changes with sufficient advance notice and migration paths. Early adopters are welcome and encouraged to use it, and we go out of our way to make their lives easier if we ever decide to change the API. You could call it "beta-quality".
  • @preview makes no claims about API stability, but users can have a certain expectation for what the behavior should be. Early adopters are taking a risk when they use it; they are on their own if we decide to change the API. You could call it "alpha-quality".
  • @internal makes no claims about either API or behavior stability; users should never call these APIs in their own code unless they are ICU or CLDR.

I would like to see different flags for all three non-@stable levels. @draft should default to on, and @internal should default to off. We can debate whether we want @preview to default to on or off.

@srl295
Copy link
Member Author

srl295 commented Aug 2, 2018

@sffc so given your comment, perhaps this PR (as is) is a good step forward - @preview is no more visible than @internal, is still behind U_HIDE_INTERNAL_API. This only improves documentation and makes no change to the visibility of anything.

@sffc
Copy link
Member

sffc commented Aug 2, 2018

SGTM; the new flag U_HIDE_PREVIEW_API can be added in a follow-up.

@srl295 srl295 requested a review from sffc August 7, 2018 20:07
@srl295
Copy link
Member Author

srl295 commented Aug 7, 2018

@sffc @jefgen i have 'verbal' feedback from you both, please review

@jefgen jefgen self-requested a review August 7, 2018 20:09
@srl295 srl295 merged commit 25f1589 into unicode-org:master Aug 7, 2018
srl295 added a commit to srl295/icu that referenced this pull request Sep 25, 2018
@srl295 srl295 deleted the at-preview-3 branch September 26, 2018 13:40
srl295 added a commit that referenced this pull request Sep 26, 2018
sffc referenced this pull request in sffc/icu Sep 26, 2018
* ICU-13685 - add a @Preview tag in Doxygen

* Change @internal to @Preview for existing tech preview sites

ICU-13685
sffc referenced this pull request in sffc/icu Sep 26, 2018
sffc referenced this pull request in sffc/icu Sep 26, 2018
* ICU-13685 - add a @Preview tag in Doxygen

* Change @internal to @Preview for existing tech preview sites

ICU-13685
sffc referenced this pull request in sffc/icu Sep 26, 2018
sffc referenced this pull request in sffc/icu Sep 27, 2018
* ICU-13685 - add a @Preview tag in Doxygen

* Change @internal to @Preview for existing tech preview sites

ICU-13685
sffc referenced this pull request in sffc/icu Sep 27, 2018
sffc referenced this pull request in sffc/icu Sep 27, 2018
sffc referenced this pull request in sffc/icu Sep 27, 2018
* ICU-13685 - add a @Preview tag in Doxygen

* Change @internal to @Preview for existing tech preview sites

ICU-13685
sffc referenced this pull request in sffc/icu Sep 27, 2018
sffc referenced this pull request in sffc/icu Sep 27, 2018
sffc referenced this pull request in sffc/icu Sep 27, 2018
* ICU-13685 - add a @Preview tag in Doxygen

* Change @internal to @Preview for existing tech preview sites

ICU-13685
sffc referenced this pull request in sffc/icu Sep 27, 2018
sffc referenced this pull request in sffc/icu Sep 27, 2018
* ICU-13685 - add a @Preview tag in Doxygen

* Change @internal to @Preview for existing tech preview sites

ICU-13685
sffc referenced this pull request in sffc/icu Sep 27, 2018
hugovdm added a commit to hugovdm/icu that referenced this pull request Jan 17, 2020
ICU-20568 Pull CLDR-13488's unit conversion data into units.txt
hugovdm pushed a commit to hugovdm/icu that referenced this pull request Mar 25, 2020
sven-oly added a commit that referenced this pull request Aug 26, 2020
Bringing this fork up to date
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants