Skip to content

Fix Dev Guide references to new locations#11906

Merged
hashhar merged 1 commit intotrinodb:masterfrom
Ordinant:bw/fix-dev-project-refs
May 13, 2022
Merged

Fix Dev Guide references to new locations#11906
hashhar merged 1 commit intotrinodb:masterfrom
Ordinant:bw/fix-dev-project-refs

Conversation

@Ordinant
Copy link
Copy Markdown
Member

@Ordinant Ordinant commented Apr 11, 2022

Description

The Developer Guide chapter has a few references to sub-projects “at the root of the Trino source tree”. But since the reorganization of that tree, these references are one layer off. Also added one missing Oxford comma.

Is this change a fix, improvement, new feature, refactoring, or other?

Fix.

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Docs only.

How would you describe this change to a non-technical end user or system administrator?

Doc correction.

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Apr 11, 2022
@Ordinant Ordinant requested a review from electrum April 11, 2022 21:57
@github-actions github-actions bot added the docs label Apr 11, 2022
@Ordinant Ordinant requested a review from aakashnand April 11, 2022 21:59
@Ordinant
Copy link
Copy Markdown
Member Author

@aakashnand, thanks for the suggestion to link to the referenced code, but even more for doing the work! I bravely clicked the Commit changes buttons here in GitHub. Can you SQUASH and merge?

@aakashnand
Copy link
Copy Markdown
Member

@Ordinant sure will do it today.

@Ordinant Ordinant force-pushed the bw/fix-dev-project-refs branch from 5b8f3c7 to aeef2f9 Compare April 14, 2022 04:15
@Ordinant
Copy link
Copy Markdown
Member Author

@aakashnand, I squashed all commits to one. Thanks for your help!

Copy link
Copy Markdown
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

I like the corrections in terms of path to modules and such. I am not sure I like linking to the source code repo. @electrum should probably chime in if we want to do that or not.

@aakashnand
Copy link
Copy Markdown
Member

@mosabua I thought it would be more better user experience to directly click the links rather than going to repo and find that file manually

@mosabua
Copy link
Copy Markdown
Member

mosabua commented Apr 15, 2022

@mosabua I thought it would be more better user experience to directly click the links rather than going to repo and find that file manually

I understand .. but I also think I disagree .. if you are reading this dev guide you would typically have the whole codebase open in an IDE already and just jump to the class with shortcut key... at least thtats what I would do.

@mosabua mosabua requested a review from hashhar May 9, 2022 17:51
Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Let's remove links to source code since they are easy to break while the class names or module names can generally be searched for and they'll lead you to correct place even if things have been renamed.

Rest of it looks good.

@Ordinant Ordinant force-pushed the bw/fix-dev-project-refs branch from aeef2f9 to f9f9f82 Compare May 12, 2022 22:58
@Ordinant Ordinant force-pushed the bw/fix-dev-project-refs branch from f9f9f82 to 473f8dd Compare May 12, 2022 23:03
Copy link
Copy Markdown
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Awesome .. merge time ;-)

Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Thanks.

@hashhar hashhar merged commit 5972ccc into trinodb:master May 13, 2022
@hashhar hashhar added the no-release-notes This pull request does not require release notes entry label May 13, 2022
@github-actions github-actions bot added this to the 381 milestone May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed docs no-release-notes This pull request does not require release notes entry

Development

Successfully merging this pull request may close these issues.

4 participants