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

[docs] Add an example to the Base.prompt docstring #43638

Merged
merged 2 commits into from
Jan 3, 2022

Conversation

DilumAluthge
Copy link
Member

No description provided.

@DilumAluthge DilumAluthge added the docs This change adds or pertains to documentation label Jan 2, 2022
@DilumAluthge DilumAluthge added merge me PR is reviewed. Merge when all tests are passing and removed merge me PR is reviewed. Merge when all tests are passing labels Jan 2, 2022
@logankilpatrick
Copy link
Member

@DilumAluthge why does prompt need to be prefixed with base?

@DilumAluthge
Copy link
Member Author

DilumAluthge commented Jan 2, 2022

prompt is not exported from Base, so you can't use it by just doing prompt; you need to do Base.prompt.

@logankilpatrick
Copy link
Member

I do not think we should export it

Can you add some context as to why? Just for my own understanding :)

@DilumAluthge
Copy link
Member Author

It just seems like a very short generic word that doesn't necessarily need to be exported. Plus Base.prompt isn't really too long to type.

I don't feel particular strongly though, so if other people really want to export prompt, I won't object.

@logankilpatrick
Copy link
Member

My only argument is to have parity with Python (where you would use input) but not sure if that is compelling for other people or not.

@DilumAluthge DilumAluthge added the merge me PR is reviewed. Merge when all tests are passing label Jan 2, 2022
@DilumAluthge
Copy link
Member Author

If, in the future, we do end up exporting prompt from Base, we would edit this example to change Base.prompt( to prompt(.

@IanButterworth
Copy link
Member

Perhaps also add Base.prompt to the docs in this PR too?

@IanButterworth
Copy link
Member

Oh I didn't see #43637

@DilumAluthge
Copy link
Member Author

Perhaps also add Base.prompt to the docs in this PR too?

#43637

I should explain my rationale for keeping this PR and #43637 separate. This PR (adding an example to the docstring) doesn't need a lot of discussion (and can be merged pretty quickly) because it doesn't change any guarantees about the public API.

In contrast, #43637 makes Base.prompt part of the public API, so I would want other people to chime in and confirm that they agree with the decision to make it part of the public API. So that PR will need more discussion (and maybe a quick run by triage) and will probably be open for a longer period of time.

@dkarrasch dkarrasch merged commit fbe37e1 into master Jan 3, 2022
@dkarrasch dkarrasch deleted the dpa/example-for-base-prompt branch January 3, 2022 11:17
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Jan 4, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants