Skip to content

finalize terminal disablePersistence API#141898

Merged
meganrogge merged 4 commits intomainfrom
merogge/finalize-disablePersistence
Feb 4, 2022
Merged

finalize terminal disablePersistence API#141898
meganrogge merged 4 commits intomainfrom
merogge/finalize-disablePersistence

Conversation

@meganrogge
Copy link
Collaborator

This PR fixes #118726

@meganrogge meganrogge self-assigned this Jan 31, 2022
@meganrogge meganrogge added this to the February 2022 milestone Jan 31, 2022
@meganrogge meganrogge requested review from Tyriar and jrieken January 31, 2022 19:27
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
Collaborator 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