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

4401-ZipArchive-should-not-convert-FileReference-to-paths #4402

Conversation

demarey
Copy link
Contributor

@demarey demarey commented Aug 23, 2019

Always use a file reference as input for the read stream.
Fixes #4401

@Ducasse
Copy link
Member

Ducasse commented Aug 24, 2019

I will merge it now what do you think about adding a readFromStream: method?

@Ducasse Ducasse closed this Aug 24, 2019
@Ducasse Ducasse reopened this Aug 24, 2019
@demarey demarey requested review from Ducasse and jecisc August 30, 2019 14:07
@demarey
Copy link
Contributor Author

demarey commented Aug 30, 2019

For better understanding, it is easier to review commit one by one. They all have a very limited scope.

@Ducasse
Copy link
Member

Ducasse commented Aug 30, 2019

Do I understand correctly that we do not need a
xxxFromFileNamed: to support backward compatible interface since the argument of that method was not path name but already a file?

@Ducasse Ducasse merged commit 3e93aef into pharo-project:Pharo8.0 Aug 30, 2019
@demarey
Copy link
Contributor Author

demarey commented Aug 30, 2019

xxxFromFileNamed: methods had a lot of duplicated code. They can be easily replaced by a call to xxxFrom: methods taking a stream as argument.
I put deprecations: https://github.com/pharo-project/pharo/pull/4402/files#diff-d3f332fd7c43641c2205b3e3efdc5717R1 so that users can switch.

GitHub
Always use a file reference as input for the read stream. Fixes #4401

@VincentBlondeau
Copy link
Contributor

VincentBlondeau commented Aug 30, 2019

This PR has been integrated despite it was known to break the CI:

..RETRY->BaselineOfTonel
[32] ...RETRY->BaselineOfTonelMessageNotUnderstood: ByteString>>binaryReadStream
[32] ByteString(Object)>>doesNotUnderstand: #binaryReadStream
[32] ZipArchive>>readFrom:
[32] MetacelloPharo30Platform(MetacelloPharoPlatform)>>extractRepositoryFrom:to:

[32] MCGitHubRepository class(MCGitBasedNetworkRepository class)>>projectDirectoryFrom:version:
[32] MCGitHubRepository(MCGitBasedNetworkRepository)>>calculateRepositoryDirectory
[32] MCGitHubRepository(MCGitBasedNetworkRepository)>>resolveLocalRespository
[32] MCGitHubRepository(MCGitBasedNetworkRepository)>>ensureLocalRepository
[32] MCGitHubRepository(MCGitBasedNetworkRepository)>>localRepository
[32] MCGitHubRepository(MCGitBasedNetworkRepository)>>goferReferences

And so now, the CI is broken... I suggest to revert it for now...

@demarey
Copy link
Contributor Author

demarey commented Aug 30, 2019

The fix is there: demarey@fb70a73
Indeed, the PR was integrated just before this commit.

@Ducasse
Copy link
Member

Ducasse commented Aug 31, 2019

Ok I got probably confused because normally I systematically check builds.
Tx

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.

ZipArchive should not convert FileReference to paths
3 participants