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

finalize terminal disablePersistence API #141898

Merged
merged 4 commits into from Feb 4, 2022

Conversation

meganrogge
Copy link
Contributor

This PR fixes #118726

@meganrogge meganrogge self-assigned this Jan 31, 2022
@meganrogge meganrogge added this to the February 2022 milestone Jan 31, 2022
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Think we'll need to bring it up in the API sync but this looks like what I'd expect

@jrieken
Copy link
Member

jrieken commented Feb 3, 2022

I am surprised myself but we actually haven't used the disable-prefix yet. That makes me wonder if can find an antonym of "persistence" which we use by itself or with the is-prefix, like transient?: boolean isTransient?: boolean (transient has been used for notebooks). The default would still be persisted terminals

@Tyriar
Copy link
Member

Tyriar commented Feb 3, 2022

That would mean we use a different word for the feature only in the api though?

@jrieken
Copy link
Member

jrieken commented Feb 3, 2022

Sure - I'd say that's the norm and not an exception... Settings, command ids etc are kinda API but don't go by any rules. And these names also must not always aline

@meganrogge
Copy link
Contributor Author

meganrogge commented Feb 3, 2022

@jrieken should we discuss the name at the next API sync or is this good to merge?

@Tyriar
Copy link
Member

Tyriar commented Feb 3, 2022

I see we use persistent in EnvironmentVariableCollection, but it's a value given to the extension and optional booleans in the case of this API feel wrong weird when they default to false and I don't think we do this anywhere.

isTransient seems fine to me if we can't use persistent/persistence, there's always the docs to clarify.

@meganrogge meganrogge merged commit 6b0442b into main Feb 4, 2022
@meganrogge meganrogge deleted the merogge/finalize-disablePersistence branch February 4, 2022 06:06
@jrieken
Copy link
Member

jrieken commented Feb 4, 2022

Yeah, we will probably do a quick mention at the next API sync. Tho, I don't expect this to be controversial or anyone of you to be there to "defend" it.

@andyleejordan
Copy link
Member

Thanks so much @meganrogge!!! This will really improve the user experience for a lot of extensions.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow extension-owned terminals to opt out of being a persistent terminal
5 participants