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

Few characterisation tests for ImageFactory and ImageType as enum class #732

Closed
wants to merge 14 commits into from
Closed

Few characterisation tests for ImageFactory and ImageType as enum class #732

wants to merge 14 commits into from

Conversation

piponazo
Copy link
Collaborator

@piponazo piponazo commented Mar 3, 2019

I started to analyse the core part of Exiv2 and I took the decision to start adding few characterisation tests (in form of Unit Tests) for the ImageFactory class. I soon realised about few things we could improve there and I prepared this Draft to discuss them.

First of all, I found bizarre that I had to include all the headers corresponding to the different Image Types to be able to create an instance of them. Normally, when using the Factory design pattern, the benefit of it is that you just need to know the Abstract Type (In this case Image) and you do not need to know the implementation details about each of the concrete implementations. I decided then to perform an experiment and replace the ImageType namespace that was populated in each image type header file, by a enum class type defined in a new file called image_types.hpp. The benefits from my point of view:

  • We do not need to include all the Image Type headers to use the Factory.
  • We have a unique place where the ImageType values are set.

I also started to use the doxygen style ///which I think make the documentation more readable and concise.

The other interesting thing that I noticed thanks to the tests is that we do not support the creation of Image Metadata structures for most of the formats. Yes, we can read them, but not create them from scratch ... something that is not evident with the existing documentation.

Note that I created this PR as a Draft to get your feedback about the first steps I am taking here. My idea is to create a PR with the ImageFactory fully tested.

I would recommend you to take a look commit by commit, to see the decisions I took step by step.

@piponazo piponazo added the enhancement feature / functionality enhancements label Mar 3, 2019
@piponazo piponazo self-assigned this Mar 3, 2019
tbeu
tbeu previously approved these changes Mar 3, 2019
Copy link
Member

@tbeu tbeu left a comment

Choose a reason for hiding this comment

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

I am in strong favour of introducing the type-safe enum class for the image types. But I would not have it mixed with the doxygen style changes. This should be a separate commit and considered similar as the clang-formatting changes.

@tbeu
Copy link
Member

tbeu commented Mar 3, 2019

Are you sure 6c0e96f and d3636d9 belong here?

@tbeu tbeu self-requested a review March 3, 2019 17:44
@piponazo
Copy link
Collaborator Author

piponazo commented Mar 3, 2019

Are you sure 6c0e96f and d3636d9 belong here?

I agree that they do not belong here. Note that this is a Draft PR and I am just prototyping stuff. After I get feedback from you guys, I will create clean PRs trying to focus on just 1 topic each 😉

@mergify mergify bot dismissed tbeu’s stale review March 3, 2019 17:59

Pull request has been modified.

@clanmills
Copy link
Collaborator

This all seems like a good idea to me. I like the imageType symbols are defined in an enum instead of a const int xxx=value which has occasionally caused trouble in the past. The "old" numbers in comments can be removed.

I'll approve this if you request me to do so. From your comments, I think you're only asking for feedback and will provide a "solid" PR after your vacation.

@piponazo
Copy link
Collaborator Author

piponazo commented Mar 4, 2019

I'll approve this if you request me to do so. From your comments, I think you're only asking for feedback and will provide a "solid" PR after your vacation.

Exactly. I just wanted to know if at least this seemed like a good idea to more than 1 person 😉 . I'll prepare a better PR soon.

@piponazo piponazo closed this Mar 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement feature / functionality enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants