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

Improve output channel API options #59322

Closed
joaomoreno opened this issue Sep 25, 2018 · 12 comments
Closed

Improve output channel API options #59322

joaomoreno opened this issue Sep 25, 2018 · 12 comments
Assignees
Labels
api output Output channel system issues

Comments

@joaomoreno
Copy link
Member

Testing #59238

There's a couple issues with the current API:

/**
 * Creates a new [output channel](#OutputChannel) with the given name.
 *
 * By default, Output channels use a separate transport mechanism when appending data.
 * This helps to reduce CPU load on the UI process but also means that the output channel UI updates with a small delay.
 * Use the force-flag to enforce immediate updates at the cost of higher CPU usage.
 *
 * @param name Human-readable string which will be used to represent the channel in the UI.
 * @param options Options to control how the channel will be created.
 */
export function createOutputChannel(name: string, options?: { force?: boolean }): OutputChannel;
  • The type of options should be extracted to an independent type: OutputChannelOptions.
  • The perf description is very confusing. Eg Output channels use a separate transport mechanism when appending data... separate from what? Let's go straight to the point and just mention the different options for creating output channels and their implications.
  • The description about output channels perf should be documenting the force field of the options bag, not the createOutputChannel function.
  • force is a terrible name. @jrieken can surely come up with a better abstraction, he usually does
    • Should it be a type instead? That way it'll make it easier to describe that there are two different types of output channels, each with its own perf/qos characteristics, and the caller can override from the default type, etc.
@jrieken
Copy link
Member

jrieken commented Sep 25, 2018

@joaomoreno welcome to the party: #58116 (comment)

@sandy081 sandy081 added api output Output channel system issues labels Sep 25, 2018
@sandy081 sandy081 added this to the September 2018 milestone Sep 25, 2018
@jrieken
Copy link
Member

jrieken commented Sep 25, 2018

The type of options should be extracted to an independent type: OutputChannelOptions

@sandy081 I actually don't know why we have options instead of a simple boolean?

@joaomoreno
Copy link
Member Author

joaomoreno commented Sep 25, 2018

inMemory was actually not too bad! Others:

  • transient
  • noPersist
  • persistent (defaults to true)
  • useLogFile (defaults to true)

@jrieken
Copy link
Member

jrieken commented Sep 25, 2018

Since it's not very intuitive we try to avoid something optional that defaults to true

@sandy081
Copy link
Member

@jrieken I think you had the same question during review and it is because

There is a request to set a lang id for channels. If so it has to be set while creating. Hence created options object.

@sandy081
Copy link
Member

I also think inMemory as a better option even though we use log files on the renderer side when true. When channel is open, data directly goes into in memory model where as in other case it first goes to file and then to the model.

@jrieken
Copy link
Member

jrieken commented Sep 25, 2018

other case it first goes to file and then to the model.

and that means it is not in memory?

@sandy081
Copy link
Member

Eventually, for all channels, content gets loaded in to model which is in memory to show in the UI. But the difference is that those channels with special flag, directly transmit data to renderer from ext host, in which case data remains in memory.

@jrieken
Copy link
Member

jrieken commented Sep 25, 2018

Well, you decide, these are the constraints

  • don't have something optional that default to true
  • because we want people to use the log-file use a name that kinda means 'I am doing something bad/expensive'
  • don't use a technical term, esp. not inMemory because that also shows up as scheme

Last, be aware that you created this problem just for yourself (trying to avoid a third argument in the future) and that using a parameter gives you the flexibility to change it later

@sandy081
Copy link
Member

/**
 * Options to configure the behavior of the [OutputChannel](#OutputChannel)
 */
export interface OutputChannelOptions {

	/**
	 * By default, data appended to Output channels is written into a log file and is not piped to UI process immediately.
	 * Output channel UI is updated by listening to the corresponding log file changes.
	 * This helps to reduce CPU load on the UI process but also means that the output channel UI updates with a small delay.
	 *
	 * Enabling this flag will allow the data to be piped to UI process for immediate updates at the cost of higher CPU usage.
	 */
	readonly enablePiping?: boolean

}

/**
 * Creates a new [output channel](#OutputChannel) with the given name.
 *
 * @param name Human-readable string which will be used to represent the channel in the UI.
 * @param options Options to configure the behavior of the channel.
 */
export function createOutputChannel(name: string, options?: OutputChannelOptions): OutputChannel;

What do you think?

@jrieken
Copy link
Member

jrieken commented Sep 25, 2018

What do you think?

I think you should make it a param instead of an object and pick any name because that's not set in stone

sandy081 added a commit that referenced this issue Sep 25, 2018
@sandy081
Copy link
Member

Removed the options completely.

@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api output Output channel system issues
Projects
None yet
Development

No branches or pull requests

3 participants