Skip to content

Conversation

@amoeba
Copy link
Member

@amoeba amoeba commented Feb 7, 2024

Rationale for this change

basic_arrow.rst had two minor issues:

  • It used (U+2019) instead of '
  • While editing, I found it wasn't whitespace normalized

What changes are included in this PR?

  • replaced with '
  • Whitespace has been normalized

Are these changes tested?

No

Are there any user-facing changes?

Just docs.

@amoeba amoeba changed the title MIONR: [Docs] Fix format issues in basic_arrow.rst MINOR: [Docs] Fix format issues in basic_arrow.rst Feb 7, 2024
@github-actions github-actions bot added the awaiting review Awaiting review label Feb 7, 2024
@kou kou changed the title MINOR: [Docs] Fix format issues in basic_arrow.rst MINOR: [Docs] Use ' and remove trailing spaces in basic_arrow.rst Feb 8, 2024
@kou
Copy link
Member

kou commented Feb 8, 2024

@github-actions crossbow submit preview-docs

@kou
Copy link
Member

kou commented Feb 8, 2024

Can we introduce linter/formatter for docs/?
We don't want to do this type checks manually...

@github-actions
Copy link

github-actions bot commented Feb 8, 2024

Revision: d15468a2c6a0967e649743ba7c7110c5f828e732

Submitted crossbow builds: ursacomputing/crossbow @ actions-5240b8fe97

Task Status
preview-docs GitHub Actions

@amoeba
Copy link
Member Author

amoeba commented Feb 8, 2024

Not a bad idea. The original issue I found (ambiguous unicode chars) should hopefully be very rare as characters like that only get introduced when contributors use editors not meant for source code. A check in CI for the second issue (non-normalized whitespace) would be nice. I can file an issue to look into it.

@amoeba
Copy link
Member Author

amoeba commented Feb 8, 2024

I filed #39990 and will have a look.

@amoeba
Copy link
Member Author

amoeba commented Feb 8, 2024

Hey @kou, I think sphinx-lint might work here, see my comment. I didn't find anything that can catch ambiguous Unicode characters so I still think this PR should be merged. I'll work on another PR for #39990.

@kou
Copy link
Member

kou commented Feb 8, 2024

Thanks!
It seems that sphinx-lint is a good candidate. We may want to enable "trailing whitespace" at the first step and enable other checks later.

I want to ensure whether this change doesn't introduce a syntax error before I merge this. But preview-docs is broken now. I hope that #39993 solves it. Can we rebase on main and run preview-docs again after #39993 is merged?

@amoeba
Copy link
Member Author

amoeba commented Feb 8, 2024

We may want to enable "trailing whitespace" at the first step and enable other checks later.

Works for me.

Can we rebase on main and run preview-docs again after #39993 is merged?

Of course, I'll keep an eye on that.

@kou
Copy link
Member

kou commented Feb 8, 2024

I've merged #39993. Could you rebase on main?

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Feb 8, 2024
@amoeba amoeba force-pushed the fix-basic-arrow-docs branch from d15468a to 3c7aa44 Compare February 8, 2024 18:56
@amoeba
Copy link
Member Author

amoeba commented Feb 8, 2024

@github-actions crossbow submit preview-docs

@github-actions
Copy link

github-actions bot commented Feb 8, 2024

Revision: 3c7aa44a2dada2fdd7c0ac6996d32acc54ec1c1f

Submitted crossbow builds: ursacomputing/crossbow @ actions-d426402f0d

Task Status
preview-docs GitHub Actions

@amoeba
Copy link
Member Author

amoeba commented Feb 8, 2024

Hi @kou, I've rebased and started another docs build. I tested the output locally too and it looks fine. This should be ready for another look.

@kou kou force-pushed the fix-basic-arrow-docs branch from 3c7aa44 to 571201c Compare February 9, 2024 01:21
@kou
Copy link
Member

kou commented Feb 9, 2024

@github-actions crossbow submit preview-docs

@github-actions
Copy link

github-actions bot commented Feb 9, 2024

Revision: 571201c

Submitted crossbow builds: ursacomputing/crossbow @ actions-c509de5fda

Task Status
preview-docs GitHub Actions

@kou
Copy link
Member

kou commented Feb 10, 2024

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

@kou kou merged commit d989191 into apache:main Feb 10, 2024
@kou kou removed the awaiting merge Awaiting merge label Feb 10, 2024
@github-actions github-actions bot added the awaiting merge Awaiting merge label Feb 10, 2024
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit d989191.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

@amoeba
Copy link
Member Author

amoeba commented Feb 10, 2024

Thanks @kou.

dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…ache#39989)

### Rationale for this change

basic_arrow.rst had two minor issues:
- It used `’` (U+2019) instead of `'`
- While editing, I found it wasn't whitespace normalized

### What changes are included in this PR?

- `’` replaced with `'`
- Whitespace has been normalized

### Are these changes tested?

No

### Are there any user-facing changes?

Just docs.

Authored-by: Bryce Mecum <petridish@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants