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

Simplify buildPaths function #2162

Merged
merged 1 commit into from
Oct 18, 2018
Merged

Conversation

gusty
Copy link
Member

@gusty gusty commented Oct 17, 2018

Description

  • Avoid unnecessary conversion to seq
  • Unify use of collect
  • Remove unused variables, unneeded parens

@matthid
Copy link
Member

matthid commented Oct 17, 2018

Hey thanks, good changes. Just to ensure we don't break anything. Do we have tests for this area? If not, can we add some around this code?

@gusty
Copy link
Member Author

gusty commented Oct 17, 2018

This project in general doesn't seem to have many tests.
Correct me if I'm wrong but all I see is 8 integrations tests and 0 unit tests.

@BlythMeister
Copy link
Contributor

There is quite a bit more than that. Look in src/test
Whilst there isn't great coverage, we are trying to add tests for changes to boost the set.

@matthid
Copy link
Member

matthid commented Oct 18, 2018

Yes indeed there are more and more unit tests (also "we have always done it like that" is not a good argument).

The advantage for me is that I need less time verifying or manually tesing the stuff, therefore - after the latest improvement in the core to simplify writing tests - I'm trying to focus more on tests as well (personally and for PRs)

Also I'm not sure but I believe there are already tests around globbing so if you find some tests already covering the code path that's fine and no new tests are required

@gusty
Copy link
Member Author

gusty commented Oct 18, 2018

Yes, there are tests for globbing https://github.com/fsharp/FAKE/blob/release/next/src/test/Fake.Core.UnitTests/Fake.IO.Globbing.fs but not sure if they cover the code path. I'll have to dig into it.

@gusty
Copy link
Member Author

gusty commented Oct 18, 2018

Actually I think it covers it, since buildPaths is used in search which is used in the IEnumerable<'t> interface for LazyGlobbingPattern which I see at the beginning of the test file.

@matthid matthid merged commit 25a04f3 into fsprojects:release/next Oct 18, 2018
@matthid
Copy link
Member

matthid commented Oct 18, 2018

Thanks, sorry but I'm just trying to reduce the number of manual fixups I have to do afterwards. Maybe I should also be more strict in reverting stuff if it fails after release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants