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

[Analyzer] Refacto our image analyzers to further expand the gem (#254) #299

Conversation

Mth0158
Copy link
Collaborator

@Mth0158 Mth0158 commented Nov 24, 2024

No description provided.

@Mth0158 Mth0158 force-pushed the 254-refactor-the-metadatarb-from-activestorageanalyzerimageanalyzerimagemagickvipsnewblobmetadata branch 2 times, most recently from 521f837 to 0362bc0 Compare November 24, 2024 17:12
@Mth0158 Mth0158 force-pushed the 254-refactor-the-metadatarb-from-activestorageanalyzerimageanalyzerimagemagickvipsnewblobmetadata branch from 0362bc0 to 274bcad Compare November 24, 2024 18:38
@Mth0158 Mth0158 force-pushed the 254-refactor-the-metadatarb-from-activestorageanalyzerimageanalyzerimagemagickvipsnewblobmetadata branch from fe7e3d0 to 0ef64e4 Compare November 25, 2024 13:52
@Mth0158 Mth0158 force-pushed the 254-refactor-the-metadatarb-from-activestorageanalyzerimageanalyzerimagemagickvipsnewblobmetadata branch from fa48e7e to 77491c6 Compare November 25, 2024 14:00
@Mth0158 Mth0158 mentioned this pull request Nov 25, 2024
@Mth0158 Mth0158 force-pushed the 254-refactor-the-metadatarb-from-activestorageanalyzerimageanalyzerimagemagickvipsnewblobmetadata branch 2 times, most recently from 7b06bb0 to cb2d918 Compare November 25, 2024 17:25
@Mth0158 Mth0158 force-pushed the 254-refactor-the-metadatarb-from-activestorageanalyzerimageanalyzerimagemagickvipsnewblobmetadata branch from cb2d918 to 96033ed Compare November 25, 2024 17:28
@Mth0158
Copy link
Collaborator Author

Mth0158 commented Nov 26, 2024

Self note: ensure #199 is fixed with this?

UPDATED from #199:

It should be fixed by #299 (0 byte size test case). I am closing the issue, feel free to reopen it if it's necessary. The update should be available in the coming release (1.3.5)

@Mth0158
Copy link
Collaborator Author

Mth0158 commented Nov 26, 2024

Self note: ensure #242 is handled with this?


def read_image
begin
require "mini_magick"
Copy link
Owner

Choose a reason for hiding this comment

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

question if we need to require it always here
can we just check if defined?(MiniMagick)?

and maybe raise the error even sooner, in the initializer (check when the app boots if possible)?

Copy link
Collaborator Author

@Mth0158 Mth0158 Dec 8, 2024

Choose a reason for hiding this comment

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

I agree with you, I think we do not need to require it every time. That's, indeed, not necessary, plus not doing it has performance benefits.

I am updating the code with a unless defined?(MiniMagick) (same for Vips), as it was done previouslu by the metadata.rb file (I should have ketp it).

For the second part, we could raise the error at the app boot, but I think this will enforce the user to have both imagemagick and ruby-vips gem in their Gemfile (which we do not want). Plus, if the user does not use validators based on the image analyzers, it's not necessary to have either imagemagick or ruby-vips gem. Therefore I think it's better to leave it this way? What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using Object.const_defined?(:Vips) is better than unless defined?(Vips) because we might not have loaded the lib yet, therefore it might not be defined yet. Whereas Object.const_defined?(:Vips) is about presence in the code.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm fine with bject.const_defined?(:Vips)

I'm also fine if it just raises an exception and we will have something about it in the docs. Not the most user friendly but this should be expected. Bu this option is least preferable.

Copy link
Collaborator Author

@Mth0158 Mth0158 Dec 11, 2024

Choose a reason for hiding this comment

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

@igorkasyanchuk I finally opted for defined?(xxx). It is the best option you are right. Let me know if the PR is okay with you now :)

@Mth0158 Mth0158 force-pushed the 254-refactor-the-metadatarb-from-activestorageanalyzerimageanalyzerimagemagickvipsnewblobmetadata branch 3 times, most recently from 44a6c6f to 96033ed Compare December 8, 2024 17:05
@igorkasyanchuk igorkasyanchuk merged commit 6095f2b into master Dec 11, 2024
44 checks passed
@igorkasyanchuk igorkasyanchuk deleted the 254-refactor-the-metadatarb-from-activestorageanalyzerimageanalyzerimagemagickvipsnewblobmetadata branch December 11, 2024 20:18
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.

Refactor the metadata.rb from ActiveStorage::Analyzer::ImageAnalyzer::ImageMagick/Vips.new(blob).metadata
2 participants