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

Enable hideIgnoredNames with hideVcsIgnoredFiles #1252

Merged
merged 3 commits into from Aug 17, 2018

Conversation

Projects
None yet
3 participants
@denis-sokolov
Contributor

denis-sokolov commented May 25, 2018

Description of the Change

Interpreting English, when both “hide ignored names” and “hide vcs ignored files” are enabled, I would expect both to apply, and the union of the two options of files to be hidden.

Before this commit, however, the “hide vcs ignored files” would override the “hide ignored names” for no apparent benefit.

Alternate Designs

No alternate designs were considered.

Benefits

The behavior will more closely match the user’s expectation based on the shared interface in English.

Possible Drawbacks

Some users may have been relying on the gitignored files overriding the custom “ignored names” setting.

Applicable Issues

Fixes #489

Enable hideIgnoredNames with hideVcsIgnoredFiles
Interpreting English, when both “hide ignored names” and “hide vcs ignored files” are enabled, I would expect both to apply, and the union of the two options of files to be hidden.

Before this commit, however, the “hide vcs ignored files” would override the “hide ignored names” for no apparent benefit.
@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Jul 31, 2018

Member

Fixes #489.

Could you please add some tests for this PR as well?

Member

50Wliu commented Jul 31, 2018

Fixes #489.

Could you please add some tests for this PR as well?

@50Wliu 50Wliu added the needs-review label Jul 31, 2018

@lee-dohm

This comment has been minimized.

Show comment
Hide comment
@lee-dohm

lee-dohm Jul 31, 2018

Member

As @50Wliu mentioned, we need tests to accept this PR. Additionally, with the addition of the if (expr) return true construct, this now returns undefined instead of false, which is an undesired change. We would want this to maintain the behavior of only returning true or false.

Thanks for your effort on this!

Member

lee-dohm commented Jul 31, 2018

As @50Wliu mentioned, we need tests to accept this PR. Additionally, with the addition of the if (expr) return true construct, this now returns undefined instead of false, which is an undesired change. We would want this to maintain the behavior of only returning true or false.

Thanks for your effort on this!

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Jul 31, 2018

Member

I believe with this change the logic will still pass down to the final return false call at https://github.com/atom/tree-view/pull/1252/files#diff-fa19a8d46cd6fbf5a016c0719da3b487R195, so I think the function will still only return a boolean.

Member

50Wliu commented Jul 31, 2018

I believe with this change the logic will still pass down to the final return false call at https://github.com/atom/tree-view/pull/1252/files#diff-fa19a8d46cd6fbf5a016c0719da3b487R195, so I think the function will still only return a boolean.

@denis-sokolov

This comment has been minimized.

Show comment
Hide comment
@denis-sokolov

denis-sokolov Aug 3, 2018

Contributor

@lee-dohm, not returning false and falling through to other options is the purpose of this PR.

Contributor

denis-sokolov commented Aug 3, 2018

@lee-dohm, not returning false and falling through to other options is the purpose of this PR.

@denis-sokolov

This comment has been minimized.

Show comment
Hide comment
@denis-sokolov

denis-sokolov Aug 3, 2018

Contributor

I’d be happy to add tests, but I look at the tree-view-package-spec file and it’s a bit overwhelming. :-(
https://github.com/atom/tree-view/blob/master/spec/tree-view-package-spec.coffee

Contributor

denis-sokolov commented Aug 3, 2018

I’d be happy to add tests, but I look at the tree-view-package-spec file and it’s a bit overwhelming. :-(
https://github.com/atom/tree-view/blob/master/spec/tree-view-package-spec.coffee

@lee-dohm lee-dohm added help-wanted and removed needs-review labels Aug 6, 2018

@lee-dohm

This comment has been minimized.

Show comment
Hide comment
@lee-dohm

lee-dohm Aug 6, 2018

Member

We would like to be in a position to mentor people on writing tests for things. Unfortunately, we're not currently there at this point. We've got some mid-term plans to hopefully change that but for now we're having to close PRs if they can't be supplied with tests. I have labeled the PR as help-wanted so that people searching for things that they can help with can easily find it. Keep in mind, just because we are closing it now doesn't mean that it can't be reopened by yourself or us in the future if circumstances change.

Thanks for the effort you've put into it and if there's anything I can help answer, you can find me in the Atom Slack team.

Member

lee-dohm commented Aug 6, 2018

We would like to be in a position to mentor people on writing tests for things. Unfortunately, we're not currently there at this point. We've got some mid-term plans to hopefully change that but for now we're having to close PRs if they can't be supplied with tests. I have labeled the PR as help-wanted so that people searching for things that they can help with can easily find it. Keep in mind, just because we are closing it now doesn't mean that it can't be reopened by yourself or us in the future if circumstances change.

Thanks for the effort you've put into it and if there's anything I can help answer, you can find me in the Atom Slack team.

@lee-dohm lee-dohm closed this Aug 6, 2018

@denis-sokolov

This comment has been minimized.

Show comment
Hide comment
@denis-sokolov

denis-sokolov Aug 6, 2018

Contributor

Wait, @lee-dohm. Consider that the current behavior is less meaningful and similarly untested. Choosing between two untested behaviors, would you prefer the one that is more or less functional?

Contributor

denis-sokolov commented Aug 6, 2018

Wait, @lee-dohm. Consider that the current behavior is less meaningful and similarly untested. Choosing between two untested behaviors, would you prefer the one that is more or less functional?

@50Wliu 50Wliu reopened this Aug 17, 2018

50Wliu added some commits Aug 17, 2018

@50Wliu 50Wliu removed the help-wanted label Aug 17, 2018

@50Wliu 50Wliu self-assigned this Aug 17, 2018

@50Wliu 50Wliu merged commit cae1faf into atom:master Aug 17, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Aug 17, 2018

Member

@denis-sokolov added the tests for you. Hopefully that helps give you an idea of how to write them :).

Member

50Wliu commented Aug 17, 2018

@denis-sokolov added the tests for you. Hopefully that helps give you an idea of how to write them :).

@denis-sokolov denis-sokolov deleted the denis-sokolov:patch-2 branch Aug 17, 2018

@denis-sokolov

This comment has been minimized.

Show comment
Hide comment
@denis-sokolov

denis-sokolov Aug 17, 2018

Contributor

Thanks, @50Wliu! That test commit is very helpful and straight-forward, I suppose I have the moral obligation to do #1251 now myself. :-)

If you find this feedback useful, consider that the test code itself seems reasonable when viewed as one small structural bit. If the test spec was not a single 5k-line file, but a directory structure, with submodules and 100-line files, it would be much less daunting for me as a guest contributor.

Contributor

denis-sokolov commented Aug 17, 2018

Thanks, @50Wliu! That test commit is very helpful and straight-forward, I suppose I have the moral obligation to do #1251 now myself. :-)

If you find this feedback useful, consider that the test code itself seems reasonable when viewed as one small structural bit. If the test spec was not a single 5k-line file, but a directory structure, with submodules and 100-line files, it would be much less daunting for me as a guest contributor.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Aug 18, 2018

Member

Yeah...it's kinda just grown into a very large mess that no one quite wants to refactor because of how much it is. Ctrl+F'ing is a good way to try to figure out where to put your test though.

Member

50Wliu commented Aug 18, 2018

Yeah...it's kinda just grown into a very large mess that no one quite wants to refactor because of how much it is. Ctrl+F'ing is a good way to try to figure out where to put your test though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment