Skip to content

Conversation

@bowenliang123
Copy link
Contributor

@bowenliang123 bowenliang123 commented Dec 25, 2023

🔍 Description

Issue References 🔗

As descirbed.

Describe Your Solution 🔧

Types of changes 🔖

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

Behavior Without This Pull Request ⚰️

No functional changes.

Config doc page:
image

Behavior With This Pull Request 🎉

No functional changes.

Config doc page:
image

Related Unit Tests


Checklists

📝 Author Self Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • This patch was not authored or co-authored using Generative Tooling

📝 Committer Pre-Merge Checklist

  • Pull request title is okay.
  • No license issues.
  • Milestone correctly set?
  • Test coverage is ok
  • Assignees are selected.
  • Minimum number of approvals
  • No changes are requested

Be nice. Be informative.

@github-actions github-actions bot added kind:documentation Documentation is a feature! module:common labels Dec 25, 2023
@bowenliang123 bowenliang123 changed the title Use "(none)" placeholder for default value of unset configs on confing doc page Use "(none)" placeholder for default value of un-preset configs on confing doc page Dec 25, 2023
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c290dae) 61.57% compared to head (3b141dd) 61.54%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #5916      +/-   ##
============================================
- Coverage     61.57%   61.54%   -0.03%     
  Complexity       23       23              
============================================
  Files           616      616              
  Lines         36386    36386              
  Branches       4978     4978              
============================================
- Hits          22404    22395       -9     
- Misses        11570    11572       +2     
- Partials       2412     2419       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bowenliang123 bowenliang123 marked this pull request as ready for review December 25, 2023 05:16
@bowenliang123 bowenliang123 self-assigned this Dec 25, 2023
@bowenliang123 bowenliang123 added this to the v1.9.0 milestone Dec 25, 2023
@bowenliang123 bowenliang123 deleted the none-ph branch December 25, 2023 07:37
@yaooqinn
Copy link
Member

Only affects the config doc page, no functional changes

Could this be an unnotified breaking change, for example, the return value from retrieving an unset configuration?

BTW, (none) is only manually used on Spark's doc side. For its code side, it uses <undefined>

FYI, apache/spark#19974

@yaooqinn
Copy link
Member

BTW, it seems that 📝 Committer Pre-Merge Checklist is not welcomed at all.

@bowenliang123
Copy link
Contributor Author

bowenliang123 commented Dec 28, 2023

Only affects the config doc page, no functional changes

Could this be an unnotified breaking change, for example, the return value from retrieving an unset configuration?

BTW, (none) is only manually used on Spark's doc side. For its code side, it uses <undefined>

FYI, apache/spark#19974

Let me think again. And if you have worries on this PR, feel free to revert it any time.

@yaooqinn
Copy link
Member

If it's only related to doc change, I guess we can either optimize the CSS style or map to (none) in the test scope for doc generation. Let's revert it first as it might contain unexpected behaviors

yaooqinn added a commit to yaooqinn/incubator-kyuubi that referenced this pull request Dec 28, 2023
@yaooqinn yaooqinn mentioned this pull request Dec 28, 2023
18 tasks
pan3793 pushed a commit that referenced this pull request Dec 29, 2023
# 🔍 Description
## Issue References 🔗

This pull request fixes #5916 as a revert

## Describe Your Solution 🔧

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

## Types of changes 🔖

- [x] Bugfix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Test Plan 🧪

#### Behavior Without This Pull Request ⚰️

#### Behavior With This Pull Request 🎉

#### Related Unit Tests

---

# Checklists
## 📝 Author Self Checklist

- [x] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project
- [x] I have performed a self-review
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my feature works
- [x] New and existing unit tests pass locally with my changes
- [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)

## 📝 Committer Pre-Merge Checklist

- [x] Pull request title is okay.
- [x] No license issues.
- [x] Milestone correctly set?
- [x] Test coverage is ok
- [x] Assignees are selected.
- [x] Minimum number of approvals
- [x] No changes are requested

**Be nice. Be informative.**

Closes #5928 from yaooqinn/revert.

Closes #5916

5c43c02 [Kent Yao] [KYUUBI #5916] Revert #5916

Authored-by: Kent Yao <yao@apache.org>
Signed-off-by: Cheng Pan <chengpan@apache.org>
zhaohehuhu pushed a commit to zhaohehuhu/incubator-kyuubi that referenced this pull request Mar 21, 2024
…-preset configs on confing doc page

# 🔍 Description
## Issue References 🔗

As descirbed.

## Describe Your Solution 🔧

- Replace the placeholder for default value of un-preset configs on doc page, changed from `<undefined>` to `(none)`
- The `(none)` placeholder style is also used in the docs of
    - Spark : https://spark.apache.org/docs/latest/configuration.html
    - Flink : https://nightlies.apache.org/flink/flink-docs-master/docs/deployment/config/
    - Trino (as `NONE`): https://trino.io/docs/current/admin/properties.html
- Only affects the config doc page, no functional changes.

## Types of changes 🔖

- [ ] Bugfix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Test Plan 🧪

#### Behavior Without This Pull Request ⚰️
No functional changes.

Config doc page:
![image](https://github.com/apache/kyuubi/assets/1935105/46c40f03-d29e-4237-a0b9-a8447f032769)

#### Behavior With This Pull Request 🎉
No functional changes.

Config doc page:
![image](https://github.com/apache/kyuubi/assets/1935105/01e3a079-2ea3-405d-8346-ea4e75606ee7)

#### Related Unit Tests

---

# Checklists
## 📝 Author Self Checklist

- [x] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project
- [x] I have performed a self-review
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have made corresponding changes to the documentation
- [ ] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my feature works
- [x] New and existing unit tests pass locally with my changes
- [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)

## 📝 Committer Pre-Merge Checklist

- [ ] Pull request title is okay.
- [ ] No license issues.
- [ ] Milestone correctly set?
- [ ] Test coverage is ok
- [ ] Assignees are selected.
- [ ] Minimum number of approvals
- [ ] No changes are requested

**Be nice. Be informative.**

Closes apache#5916 from bowenliang123/none-ph.

Closes apache#5916

3b141dd [Bowen Liang] update test
04cf8cd [Bowen Liang] Use "(none)" placeholder for default value of unset configs on config page

Authored-by: Bowen Liang <liangbowen@gf.com.cn>
Signed-off-by: Bowen Liang <liangbowen@gf.com.cn>
zhaohehuhu pushed a commit to zhaohehuhu/incubator-kyuubi that referenced this pull request Mar 21, 2024
# 🔍 Description
## Issue References 🔗

This pull request fixes apache#5916 as a revert

## Describe Your Solution 🔧

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

## Types of changes 🔖

- [x] Bugfix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Test Plan 🧪

#### Behavior Without This Pull Request ⚰️

#### Behavior With This Pull Request 🎉

#### Related Unit Tests

---

# Checklists
## 📝 Author Self Checklist

- [x] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project
- [x] I have performed a self-review
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my feature works
- [x] New and existing unit tests pass locally with my changes
- [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)

## 📝 Committer Pre-Merge Checklist

- [x] Pull request title is okay.
- [x] No license issues.
- [x] Milestone correctly set?
- [x] Test coverage is ok
- [x] Assignees are selected.
- [x] Minimum number of approvals
- [x] No changes are requested

**Be nice. Be informative.**

Closes apache#5928 from yaooqinn/revert.

Closes apache#5916

5c43c02 [Kent Yao] [KYUUBI apache#5916] Revert apache#5916

Authored-by: Kent Yao <yao@apache.org>
Signed-off-by: Cheng Pan <chengpan@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:documentation Documentation is a feature! module:common

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants