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

Be more strict about multiple indexes found in an archive #175

Merged
merged 3 commits into from
Jul 13, 2019
Merged

Be more strict about multiple indexes found in an archive #175

merged 3 commits into from
Jul 13, 2019

Conversation

raindev
Copy link
Contributor

@raindev raindev commented Jul 12, 2019

Fail fast rather than attempting to parse and overwriting already initialized symbol index. Came up in
#174 (comment).

@philipc
Copy link
Collaborator

philipc commented Jul 13, 2019

This is basically fine, but you could consider using a single enum instead of multiple bools, and exposing that enum value could be useful too if users want to know which type of archive it is.

@raindev
Copy link
Contributor Author

raindev commented Jul 13, 2019

Using an enum to represent the state machine is definitely a clearer approach.

As for exposing the enum to the caller, I see the following options:

  • change the return type of Archive::parse to (ArchiveType, Result<Archive<'a>>)
  • include type: IndexType into Archive struct
  • change Archive itself to be a enum

The second option seems preferable since the first incurs a breaking change. The third seems compelling conceptually since it'll allow to expose different data present in different archives in a type safe way while maintaining a common interface but it's quite a large change which includes many breaking changes to the interface of the library.

@raindev raindev marked this pull request as ready for review July 13, 2019 08:29
@philipc
Copy link
Collaborator

philipc commented Jul 13, 2019

I prefer the second option. It makes sense to me for this to be an Archive property, rather than an additional return value, and I think changing Archive to an enum would lead to too much duplication.

src/archive/mod.rs Outdated Show resolved Hide resolved
src/archive/mod.rs Outdated Show resolved Hide resolved
src/archive/mod.rs Outdated Show resolved Hide resolved
src/archive/mod.rs Show resolved Hide resolved
Copy link
Collaborator

@philipc philipc left a comment

Choose a reason for hiding this comment

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

Thanks!

@philipc philipc merged commit 4381f16 into m4b:master Jul 13, 2019
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