Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Adds configuration option for large file warning threshold. #12439

Merged
merged 6 commits into from Aug 18, 2016
Merged

Adds configuration option for large file warning threshold. #12439

merged 6 commits into from Aug 18, 2016

Conversation

remexre
Copy link
Contributor

@remexre remexre commented Aug 16, 2016

See #10086.

@50Wliu
Copy link
Contributor

50Wliu commented Aug 16, 2016

/cc @nathansobo is this something you're ok with being customizable?

@nathansobo
Copy link
Contributor

@50Wliu Yeah I'm cool with it. Makes sense. If someone can verify this works correctly I'm 👍 to merge.

@50Wliu
Copy link
Contributor

50Wliu commented Aug 17, 2016

I tested this out and it works correctly. One thing though @remexre...can you please adjust this spec to respect the new config option and also to test out a different value to make sure that the option doesn't regress? Thanks!

@remexre
Copy link
Contributor Author

remexre commented Aug 17, 2016

Like so?

@nathansobo
Copy link
Contributor

@remexre I don't think that's quite it. What we need is a test that the limit is configurable. So you would assign a limit to the configuration in the test to a number below the mocked file size and confirm we show the dialog, then reassign the limit to a number above the mocked file size and confirm we don't show the dialog.

@remexre
Copy link
Contributor Author

remexre commented Aug 17, 2016

Is this good?

test 20, false
it "prompts for smaller files with a lower limit", ->
atom.config.set "core.warnOnLargeFileLimit", 5
test 10, true
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you space this stuff out?

@nathansobo
Copy link
Contributor

Getting close. I left a few more comments.

@remexre
Copy link
Contributor Author

remexre commented Aug 17, 2016

First two done, not 100% sure how to do the last one.

EDIT Actually, would it just be

if shouldPrompt
  # ...
else
  runs ->
    expect(editor).not.toBeUndefined()

?

@nathansobo
Copy link
Contributor

You can also do a negative assertion on your mock...

expect(atom.applicationDelegate.confirm).not.toHaveBeenCalled()

@remexre
Copy link
Contributor Author

remexre commented Aug 17, 2016

I think it's fixed now? For some reason, script/test fails, claiming it doesn't have dpkg, so local tests aren't working for me...

@50Wliu
Copy link
Contributor

50Wliu commented Aug 18, 2016

@remexre You can view specs by clicking on the ci/circleci check. Right now there's just an issue where selectedButtonIndex is undefined. In addition, I don't think there will be an available application delegate to confirm() if the file size is beneath the limit?

@50Wliu
Copy link
Contributor

50Wliu commented Aug 18, 2016

This looks good now - thanks!

@50Wliu 50Wliu merged commit af48a08 into atom:master Aug 18, 2016
@nathansobo
Copy link
Contributor

⚡️ Thanks @remexre!

@Ben3eeE
Copy link
Contributor

Ben3eeE commented Sep 28, 2016

Closes atom/tree-view#810?

@remexre
Copy link
Contributor Author

remexre commented Sep 28, 2016

I believe so.

@adamreisnz
Copy link

@remexre does this apply to actually opening files (e.g. double click), or also when previewing files from tree view (single click)? I just had the editor hang on me because I accidentally clicked on a file with 70k+ lines.

@remexre
Copy link
Contributor Author

remexre commented Oct 5, 2016

IIRC Previewing, I originally made it because I was annoyed by misclicking a.out instead of asourcefile.c.

@adamreisnz
Copy link

Great, thanks

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.

None yet

6 participants