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

use a Dry::Struct/Types default for FileMetadata type attribute #4226

Closed
elrayle opened this issue Jan 28, 2020 · 4 comments · Fixed by #4215
Closed

use a Dry::Struct/Types default for FileMetadata type attribute #4226

elrayle opened this issue Jan 28, 2020 · 4 comments · Fixed by #4215

Comments

@elrayle
Copy link
Contributor

elrayle commented Jan 28, 2020

Descriptive summary

In file app/models/hyrax/file_metadata.rb, method #for sets the default of type to Hyrax::FileSet::ORIGINAL_FILE. It is preferred to use a Dry::Struct/Types default in the type definition instead of in the #for method.

This would look something like...

attribute :type, ::Valkyrie::Types::Set, default: [Hyrax::FileSet.original_file_use]

Need to determine the impact this will have on setting the type in the #for method.

Related work

PR #4225 Requested change

@no-reply
Copy link
Contributor

no-reply commented Jan 28, 2020

@elrayle are there any cases where we want to use .for to make anything other than an original_file?

EDIT:
I guess another way of asking this is when will an ActionDispatch::Http::UploadedFile have a #type? I don't see any cases in the existing tests where that's the case (i could definitely be missing something). Wondering whether we can just drop the whole line in .for.

@elrayle
Copy link
Contributor Author

elrayle commented Jan 28, 2020

Short answer: I am fine with dropping .for. We can always add it back in if it is needed later.

Longer answer:

FileMetadata was originally created to follow the same conventions set out in figgy's FileMetadata. The .for method is calling in 5 places in figgy, 2 of which are from tests.

The only place I see FileMetadata.for in use is in Hyrax is in file_actor_spec

I believe the original thought was that it might be useful in a later part of the valkyrization process, e.g. when the derivative creation process was converted to use valkyrie. But any such use is hypothetical at this point in time.

@no-reply
Copy link
Contributor

@elrayle i think you've misinterpreted me. i like .for, i'm asking about the .try(:type) || [default] pattern in it.

as far as i can tell, the documented supported parameter never has #type method, so will always use the default. if that's the case, we can just use a Dry::Struct style default and drop the line that tries .try(:type)

@no-reply
Copy link
Contributor

after a conversation in slack, we've agreed to go ahead and drop .for which is only used for convenience in tests

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 a pull request may close this issue.

2 participants