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

Add support for contentFiles to Fake.DotNet.NuGet packaging #2165

Merged
merged 5 commits into from
Oct 10, 2019
Merged

Add support for contentFiles to Fake.DotNet.NuGet packaging #2165

merged 5 commits into from
Oct 10, 2019

Conversation

chappoo
Copy link
Contributor

@chappoo chappoo commented Oct 22, 2018

Fixes #2144

@chappoo
Copy link
Contributor Author

chappoo commented Oct 22, 2018

@matthid As per contribution docs, I branched from my fork origin master branch and created this PR to base on upstream master. Is that correct? I've seen other branches in upstream that have been branched / base off release/next.

@matthid matthid changed the base branch from master to release/next October 22, 2018 12:44
@matthid
Copy link
Member

matthid commented Oct 22, 2018

@chappoo Usually the diff between master and release/next is not too big and github allows to change the target branch (and I already did) so not an issue. But where are the docs because currently targeting release/next is preferred...

@chappoo
Copy link
Contributor Author

chappoo commented Oct 22, 2018

@matthid I was looking here: http://fake.build/contributing.html#Programming

This is linked to from the main repo README

@matthid
Copy link
Member

matthid commented Oct 22, 2018

@chappoo Thanks will try to update. Can you rebase or push an empty commit to re-trigger the CI processes (seems to have problems with changed branch target, maybe I was too fast)

@chappoo
Copy link
Contributor Author

chappoo commented Oct 22, 2018

@matthid Done - rebased on release/next

@matthid
Copy link
Member

matthid commented Oct 22, 2018

No idea why this pr doesn't trigger any CI (we have many so I'd assume github is to blame)... Maybe close and add a new one?

@matthid
Copy link
Member

matthid commented Oct 22, 2018

Ok it looks like they have some problems: https://status.github.com/messages

@chappoo
Copy link
Contributor Author

chappoo commented Oct 22, 2018

I saw the data consistency outage earlier - I think they had stopped all web-hooks as a precaution

@chappoo
Copy link
Contributor Author

chappoo commented Oct 22, 2018

I might start again once they've fully resumed, will kill the PR / branch and start again, not sure I can open a new PR against a branch with a closed PR?

@matthid
Copy link
Member

matthid commented Oct 22, 2018

@chappoo Yes I think that's not a problem, you can push to the same branch after a previous PR has been merged and start a new PR (in fact that's what I'm doing all the time with the release/next branch on master)

@chappoo
Copy link
Contributor Author

chappoo commented Oct 22, 2018

Looks like CI has magically started now

@matthid
Copy link
Member

matthid commented Oct 23, 2018

I think a small unit test for the new parameter would make sense

@chappoo
Copy link
Contributor Author

chappoo commented Oct 24, 2018

I think a small unit test for the new parameter would make sense

I totally agree, I'm not familiar with the test setup here though. Are there existing unit tests for the NuGet module?

@matthid
Copy link
Member

matthid commented Oct 24, 2018

We recently added a more testable API (CreateProcess) and started to switch to it when new features are added (see https://github.com/fsharp/FAKE/pull/2120/files for example). The switch should be quite straight-forward and the latest documentation only actually lists the new API.

Once the module uses CreateProcess you would refactor an internal api which returns the CreateProcess<T> instance and use that for testing (see the other PR for details).

Let me know if that helps.

@matthid matthid added the needs-tests This PR needs tests in order to be accepted label Oct 25, 2018
@matthid matthid added the waiting for author Some information or action was requested and it needs to be addressed. Or a response from author label Nov 14, 2018
@matthid
Copy link
Member

matthid commented Nov 18, 2018

any interest in finishing this?

@chappoo
Copy link
Contributor Author

chappoo commented Nov 18, 2018 via email

@matthid matthid merged commit a23a6a1 into fsprojects:release/next Oct 10, 2019
@matthid matthid mentioned this pull request Oct 12, 2019
@chappoo
Copy link
Contributor Author

chappoo commented Oct 12, 2019

@matthid The notification for this merge reminded me of this (old!) PR... appreciate the acceptance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-tests This PR needs tests in order to be accepted waiting for author Some information or action was requested and it needs to be addressed. Or a response from author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fake.DotNet.NuGet needs to support ContentFiles section in nuspec
2 participants