Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Upstream says don't directly call concat::setup #71

Merged
merged 5 commits into from
May 19, 2014
Merged

Upstream says don't directly call concat::setup #71

merged 5 commits into from
May 19, 2014

Conversation

BillWeiss
Copy link
Contributor

On ~modern versions of concat, concat::setup isn't to be called directly, and using it logs this warning:

Warning: Scope(Class[Concat::Setup]): concat::setup is deprecated as a public API of the concat module and should no longer be directly included in the manifest.

Concat 1.0.0 removed the need to include concat::setup, and 1.1.0 added the deprecation warning.

I removed a test that relied on this, couldn't think of where to add some, but I could if you can think of them :)

@luxflux
Copy link
Contributor

luxflux commented May 19, 2014

Thanks for pointing this out and fixing!

Rather than removing the test, could you replace the should with should_not? 😄

Also, could you update the Modulefile with the correct dependencies?

@luxflux
Copy link
Contributor

luxflux commented May 19, 2014

Ah, just saw that the Modulefile is ok.

@BillWeiss
Copy link
Contributor Author

Sure! Adding that test now.

@BillWeiss
Copy link
Contributor Author

Oh, no, that won't work. concat will implicitly do concat::setup, so the class exists, we just don't include it. Hmm...

I guess we could leave the test in place, since that makes sure the concats are working?

@BillWeiss
Copy link
Contributor Author

I think we should leave the test out. We're already testing the concats happening, concat::setup is an implementation detail of concat that we don't care about.

@luxflux
Copy link
Contributor

luxflux commented May 19, 2014

Yeah, no need to test internals of another library. Sorry for misleading the should_not, could have thought about this :/

Thanks for your work!

luxflux added a commit that referenced this pull request May 19, 2014
Upstream says don't directly call concat::setup
@luxflux luxflux merged commit 2daed6b into voxpupuli:master May 19, 2014
luxflux added a commit that referenced this pull request May 19, 2014
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants