Skip to content

fix: Better handling for Arrow type strings in docs #896

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.

Sign up for GitHub

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
Merged
kodiakhq merged 9 commits into from
May 24, 2023
Merged

fix: Better handling for Arrow type strings in docs #896

kodiakhq merged 9 commits into from
May 24, 2023

Conversation

@hermanschaaf
Copy link
Member

@hermanschaaf hermanschaaf commented May 22, 2023 •
edited
Loading

This fixes the rendering of some Arrow types in the docs, as well as in the JSON

@hermanschaaf hermanschaaf requested a review from yevgenypats as a code owner May 22, 2023 09:15
@github-actions github-actions bot added the fix label May 22, 2023
@hermanschaaf hermanschaaf mentioned this pull request May 22, 2023
Merged
@codecov
Copy link

codecov bot commented May 22, 2023 •
edited
Loading

Codecov Report

Patch coverage: 58.33% and project coverage change: -0.01 ⚠️

Comparison is base (2884bed) 47.28% compared to head (febec9a) 47.27%.

❗ Current head febec9a differs from pull request most recent head bbce4d2. Consider uploading reports for the commit bbce4d2 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #896      +/-   ##
==========================================
- Coverage   47.28%   47.27%   -0.01%     
==========================================
  Files          55       55              
  Lines        5025     5028       +3     
==========================================
+ Hits         2376     2377       +1     
- Misses       2399     2401       +2     
  Partials      250      250
Impacted Files Coverage Δ
types/inet.go 55.24% <0.00%> (-0.79%) ⬇️
types/json.go 60.58% <0.00%> (ø)
plugins/source/docs.go 71.64% <87.50%> (+0.21%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link

github-actions bot commented May 22, 2023 •
edited
Loading

⏱️ Benchmark results

  • DefaultConcurrencyDFS-2 resources/s: 10,553
  • DefaultConcurrencyRoundRobin-2 resources/s: 12,389
  • Glob-2 ns/op: 189.1
  • TablesWithChildrenDFS-2 resources/s: 25,239
  • TablesWithChildrenRoundRobin-2 resources/s: 27,703
  • TablesWithRateLimitingDFS-2 resources/s: 28.31
  • TablesWithRateLimitingRoundRobin-2 resources/s: 839.1

candiduslynx
candiduslynx suggested changes May 22, 2023
View reviewed changes
Copy link
Contributor

@candiduslynx candiduslynx left a comment

Choose a reason for hiding this comment

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

I do feel that the Arrow Types are OK in terms of defining what is actually going on.
So, maybe just do an upstream PR to remove the timezone if the tz is UTC? (it's the default, so no need to actually print it out if not explicitly asked for)

Additionally, there's an issue with unhandled nested type nullability in this proposal.

@hermanschaaf
Copy link
Member Author

hermanschaaf commented May 22, 2023

@candiduslynx Updated. The main thing I wanted to fix was that JSON was being printed as extension_type<fixed_byte_list> or something along those lines; turns out this was part of its String() definition, so I could fix it there. I don't love the Arrow String() implementation, as it seems inconsistent between nested types and utf8 is not as good a name as string, but I think we can live with these things for now.

@hermanschaaf hermanschaaf requested a review from candiduslynx May 22, 2023 11:45
candiduslynx
candiduslynx suggested changes May 22, 2023
View reviewed changes
@hermanschaaf hermanschaaf requested a review from candiduslynx May 22, 2023 12:21
@hermanschaaf hermanschaaf changed the title fix: Custom handling for Arrow type strings in docs fix: Better handling for Arrow type strings in docs May 24, 2023
@github-actions github-actions bot added fix and removed fix labels May 24, 2023
@kodiakhq kodiakhq bot merged commit 78699f4 into main May 24, 2023
@kodiakhq kodiakhq bot deleted the type-docs branch May 24, 2023 07:45
@cq-bot cq-bot mentioned this pull request May 24, 2023
Merged
kodiakhq bot pushed a commit that referenced this pull request May 24, 2023
@cq-bot
chore(main): Release v3.6.3 (#900)
f5f7cd7 
🤖 I have created a release *beep* *boop*
---


## [3.6.3](v3.6.2...v3.6.3) (2023-05-24)


### Bug Fixes

* Better handling for Arrow type strings in docs ([#896](#896)) ([78699f4](78699f4))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@disq disq disq approved these changes

+2 more reviewers

@candiduslynx candiduslynx candiduslynx approved these changes

@yevgenypats yevgenypats yevgenypats approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

automerge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@hermanschaaf @disq @candiduslynx @yevgenypats