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.

Sign up for GitHub

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

Jump to bottom

Add class and file filter parameters to Fake.Testing.ReportGenerator #2120

Merged
merged 2 commits into from
Oct 11, 2018
Merged

Add class and file filter parameters to Fake.Testing.ReportGenerator #2120

merged 2 commits into from
Oct 11, 2018

Conversation

magicmonty
Copy link
Contributor

Description

The ReportGenerator has added File and class filters. This PR will reflect these change with the addition of two additional optional parameters ClassFilter and FileFilter

TODO

  • New (API-)documentation for new features exist
  • no unit or integration test, doesn't make sense, as it would add a dependency to the external tool
  • Fake 5 API guideline is honored

@magicmonty
Copy link
Contributor Author

The failing on CircleCI seems to have nothing to do with this change:
It fails at Fake.DotNet.MSBuild.IntegrationTests

matthid
matthid reviewed Oct 5, 2018
View reviewed changes
src/app/Fake.Testing.ReportGenerator/ReportGenerator.fs Outdated
|> StringBuilder.append (sprintf "-reports:%s" (String.Join(";", reports)))
|> StringBuilder.append (sprintf "-targetdir:%s" parameters.TargetDir)
|> StringBuilder.appendWithoutQuotes (sprintf "-reports:%s" (String.Join(";", reports)))
|> StringBuilder.appendWithoutQuotes (sprintf "-targetdir:\"%s\"" parameters.TargetDir)
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer building a sequence/array and then using Args.toWindowsCommandLine which will handle escaping properly (which your quotes are not, for example when TargetDir ends with a backslack)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh cool, that is one I didn't know of. I will fix it

@matthid matthid closed this Oct 6, 2018
@matthid matthid reopened this Oct 6, 2018
@magicmonty
Copy link
Contributor Author

@matthid Changed the Argument generation

@matthid
Copy link
Member

matthid commented Oct 8, 2018

no unit or integration test, doesn't make sense, as it would add a dependency to the external tool

@magicmonty just to let you know we are discussing this #2114 (comment) I might come up with some idea on how to add some rudimentary unit tests and might ask you to implement some. But if you have an idea feel free to share :)

Otherwise this looks good. I'd assume circleci will go green when merging latest release/next (but no need to do so)

matthid
matthid previously approved these changes Oct 8, 2018
View reviewed changes
@magicmonty
Copy link
Contributor Author

@matthid I'm happy to add tests, but currently I have no Idea how without adding additional dependencies here. I have a little test project to test this stuff out manually but this would add a dependency on ReportGenerator to this project

@matthid
Copy link
Member

matthid commented Oct 10, 2018

@magicmonty please take a look at the place/test I commented, feel free to ask if you need any help

@matthid
Copy link
Member

matthid commented Oct 10, 2018

Hm, I tried to merge in latest release/next changes into this PR and now apparently github cannot handle the diff anymore :(

@magicmonty
Copy link
Contributor Author

I'll make a rebase on release/next if this is ok

@matthid
Copy link
Member

matthid commented Oct 11, 2018

Yes, sorry for the inconvenience...

@magicmonty
Copy link
Contributor Author

@matthid Added Unit Tests

matthid
matthid approved these changes Oct 11, 2018
View reviewed changes
Copy link
Member

@matthid matthid left a comment

Choose a reason for hiding this comment

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

Looks good, the tests fail because the helper did not support a variable number of arguments yet

@matthid matthid merged commit b2854dc into fsprojects:release/next Oct 11, 2018
@matthid
Copy link
Member

matthid commented Oct 11, 2018

Thanks!

@magicmonty magicmonty deleted the ReportGenerator branch October 12, 2018 05:39
@matthid matthid mentioned this pull request Oct 12, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthid matthid matthid approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@magicmonty @matthid