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 FileWrapper to OLEFile #9

Merged
merged 10 commits into from
Jan 18, 2021
Merged

Add support for FileWrapper to OLEFile #9

merged 10 commits into from
Jan 18, 2021

Conversation

sboh1214
Copy link
Collaborator

Hello, @MaxDesiatov ! Your library is giving me a lot of support for developing HWP File parsing library!

By the way, a problem occurred when I started developing SwiftUI document-based app.
It should get file information from FileWrapper in Foudation Framework.
It's critical because we can't get file path from it.

So, I created OLEFile2, which have fileWrapper property instead of fileHandle. And appended some support code.
Also, I checked it parse same result between OLEFile and OLEFile2.

However, You are core maintainer and I'm just freshmen in univ, so I need your kind advice. 😃
Would it be good to add another class like this?

Thanks!

@sboh1214 sboh1214 marked this pull request as ready for review January 17, 2021 07:12
@sboh1214
Copy link
Collaborator Author

Hmmm, we can't support Linux if the library uses FileWrapper.......
CI / xcode-11_0 (pull_request) and CI / xcode-11_1 (pull_request) failed because it removed from macos github-hosted runner, I think. macos github-hosted runner

@sboh1214
Copy link
Collaborator Author

  • I added some preprocessors that checks Linux.

Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Overall I think this approach leads to code duplication, which means we'd have much more code to maintain.

We already have protocol Reader declared, I think we should consider the code in extension FileHandle to be generalized as extension Reader instead. Then firstly we don't need a separate OLEFile2 class, and secondly we could add just another init to OleFile that takes Data instance as an argument and create a DataReader instance from it.

The fileHandle property should then be changed to let reader: Reader, to which we could assign either a FileHandle or a DataReader instance, depending on which is more convenient.

Does that make sense?

@MaxDesiatov
Copy link
Collaborator

MaxDesiatov commented Jan 17, 2021

I've removed Xcode 11.0 and 11.1 jobs from the workflow in the main branch. If you merge main into your branch, those checks should no longer run.

@sboh1214
Copy link
Collaborator Author

sboh1214 commented Jan 17, 2021

Thank @MaxDesiatov for your kind advice! Maybe the code was messy at first because I worked at 4 am lol.

As your suggestion, I integrated OLEFile2 with var reader: Reader.
Yes, it works fine, but there is some weird part in extension FileWrapper because it should conform to Reader.
Its Permalink
Could you check that part?

Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Amazing work, thank you!

@MaxDesiatov MaxDesiatov changed the title Add OLEFile2.init(FileWrapper) Add support for FileWrapper to OLEFile Jan 18, 2021
@MaxDesiatov
Copy link
Collaborator

MaxDesiatov commented Jan 18, 2021

@sboh1214 I've invited you as a collaborator to the project, feel free to accept the invitation and merge the PR when CI passes 👏

@sboh1214
Copy link
Collaborator Author

sboh1214 commented Jan 18, 2021

Thank you so much @MaxDesiatov !
Could you tag 0.3.0 and publish the release?

@sboh1214 sboh1214 merged commit 8711c9f into CoreOffice:main Jan 18, 2021
@sboh1214 sboh1214 deleted the file-wrapper branch January 18, 2021 15:08
@MaxDesiatov
Copy link
Collaborator

Sure, but please update CHANGELOG.md first with GitBuddy before tagging the release. You'd launch it like this:

GITBUDDY_ACCESS_TOKEN='sboh1214:REPLACE_WITH_YOUR_GITHUB_TOKEN' gitbuddy changelog --sections -b main

Then add a section for the new version with the current date to CHANGELOG.md and paste the output of the gitbuddy changelog command. You'd create a PR with the changelog update then, which is just a release formality 🙂

Also, this changelog snippet can be added to the release on GitHub itself.

@MaxDesiatov
Copy link
Collaborator

MaxDesiatov commented Jan 18, 2021

If you have any problems with the changelog update, please let me know. I think it's useful for all maintainers on a project to be aware of release processes. Or I can do this myself in the next couple of days, I'm sorry I'm a bit busy right now. Thank you!

@MaxDesiatov
Copy link
Collaborator

Ah, sorry, you've explicitly asked to me to tag and release. I'll do it, sorry about the confusion 🤦‍♂️

@sboh1214
Copy link
Collaborator Author

sboh1214 commented Jan 18, 2021

@MaxDesiatov No, it's okay! 😃 I edited it to request to you because I don't know about the release process. But also thank you for letting me know how to release it. Please release OLEKit in your spare time, not hurry.

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.

2 participants