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

Configure excludes polish #4256

Closed
isidorn opened this issue Mar 15, 2016 · 12 comments
Closed

Configure excludes polish #4256

isidorn opened this issue Mar 15, 2016 · 12 comments
Assignees
Labels
javascript JavaScript support issues polish Cleanup and polish issue verified Verification succeeded
Milestone

Comments

@isidorn
Copy link
Contributor

isidorn commented Mar 15, 2016

Related to #4007

Steps to Reproduce:

  1. Have a large workspace, generate a jsconfig.json that does not exclude anything, click on Configure Excludes in the status bar
  2. You get the attached message
  3. I propose the following changes:
  • Rename "Configure excludes..." action to "Configure Excludes"
  • In this particular case the suggestion is not very precise, why are we tryign to be smart here, the user should decide what to exclude himself imho
  • The title of the project seems to be wrong - why not use the same we show in the explorer header, in this case 'vscode'

screen shot 2016-03-15 at 15 35 26

@isidorn isidorn added javascript JavaScript support issues polish Cleanup and polish issue labels Mar 15, 2016
@egamma
Copy link
Member

egamma commented Mar 15, 2016

👍
While the algorithm to find large folders is nice. A user that configures the excludes knows where the generated js resources are. Therefore do not add the guessed folders to the message.

@egamma egamma added this to the March 2016 milestone Mar 15, 2016
@jrieken
Copy link
Member

jrieken commented Mar 15, 2016

I disagree, folks won't know to exclude things like temp folders which become giant because of a transient build step. node_modules et al are the obvious but we have been hurt more than once with those unusual folders

@jrieken
Copy link
Member

jrieken commented Mar 15, 2016

Rename "Configure excludes..." action to "Configure Excludes"

I put the ... because it opens a new UI element which I believe is the rule?

In this particular case the suggestion is not very precise, why are we tryign to be smart here, the user should decide what to exclude himself imho

see above

The title of the project seems to be wrong - why not use the same we show in the explorer header, in this case 'vscode'

Agree, it's the filename of the configuration but the action already takes you there...

@isidorn
Copy link
Contributor Author

isidorn commented Mar 15, 2016

I put the ... because it opens a new UI element which I believe is the rule?

Not sure where else in UI we use this rule? AFAIK we do not follow it in debug and task world
@stevencl opinions?

@jrieken
Copy link
Member

jrieken commented Mar 15, 2016

screen shot 2016-03-15 at 17 11 59

Like here for instance. Unsure if it's applies to message-actions?

@isidorn
Copy link
Contributor Author

isidorn commented Mar 15, 2016

Yeah i was thinking more of action labels

@jrieken
Copy link
Member

jrieken commented Mar 16, 2016

removed in the ... with #4248

@jrieken jrieken closed this as completed Mar 16, 2016
@egamma
Copy link
Member

egamma commented Mar 16, 2016

I disagree, folks won't know to exclude things like temp folders which become giant because of a transient build step. node_modules et al are the obvious but we have been hurt more than once with those unusual folders

Good point but then let's use heuristics and not show just all folders including many files. Only mention the suspects 'temp' and 'tmp' if they are big.

@jrieken
Copy link
Member

jrieken commented Mar 16, 2016

I'd approach it the other way around and not show folders like src or test and if that results to a case in which I have no folder left to propose exclusion for a generic message will show.

And on the other hand tsserver will generate a similar message which blames the last folder (https://github.com/Microsoft/TypeScript/blob/release-1.8/src/compiler/program.ts#L1117)

@egamma
Copy link
Member

egamma commented Mar 16, 2016

Excluding well known source folders like src and test works for me.

@jrieken
Copy link
Member

jrieken commented Mar 16, 2016

excluding src, test, and tests now

@jrieken jrieken closed this as completed Mar 16, 2016
@jrieken jrieken assigned isidorn and unassigned jrieken Mar 21, 2016
@isidorn isidorn added the verified Verification succeeded label Mar 23, 2016
@isidorn
Copy link
Contributor Author

isidorn commented Mar 23, 2016

Nice!

@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
javascript JavaScript support issues polish Cleanup and polish issue verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

3 participants