Simplify metric explorer getFormat#2189
Conversation
ryanrath
added
maintenance / code quality
ryanrath
requested review from
aestoltm and
connersaeli
and removed request for
connersaeli
| if (isset($request['format'])) { | ||
| $requestedFormat = strtolower($request['format']); |
There was a problem hiding this comment.
Is $request[] supposed to be $REQUEST here? I know the symfony code doesn't deal with the super global directly anymore but I think it is supposed to be that here.
There was a problem hiding this comment.
it is and I'm not sure why it was changed, I'll change it back.
| $this->request = array_merge($config, $this->request); | ||
| $requestedFormat = 'csv'; | ||
| if (isset($this->request['format'])) { | ||
| $requestedFormat = strtolower($this->request['format']); |
There was a problem hiding this comment.
What is the purpose of the strtolower call here? There is also a strtolower call in the validateFormat function.
There was a problem hiding this comment.
Just a hold over from the original code, this and the other strtolower will be removed.
| * @param array $allowedFormats | ||
| * @return string | ||
| */ | ||
| public static function validateFormat(?string $requestedFormat, string $default = 'jsonstore', array $allowedFormats = []): string |
There was a problem hiding this comment.
Why is requestedFormat allowed to be not set? What circumstances would cause it to be not set and why do we want to support this?
There was a problem hiding this comment.
I believe it was something I ran into while testing, but honestly it should never be null. I've tested removing the nullable and all tests passed so I'll be removing it
| $_REQUEST = array_merge($config, $_REQUEST); | ||
| $requestedFormat = null; | ||
| if (isset($request['format'])) { | ||
| $requestedFormat = strtolower($request['format']); |
There was a problem hiding this comment.
Same question about the strtolower call here: why is it being done when it is also called in the validateFormat function
| * @param array $allowedFormats | ||
| * @return string | ||
| */ | ||
| public static function validateFormat(?string $requestedFormat, string $default = 'jsonstore', array $allowedFormats = []): string |
There was a problem hiding this comment.
What is the design reason for having default values for the $default and the $allowedFormats parameters? Where is this function called when these defaults are used?
There was a problem hiding this comment.
Really it was just because the "ancestor" function getFormat had a $default and a $formats_subset ( which I renamed to $allowedFormats so that it made more sense. ). It's never called without a $default or a $allowedFormats argument. I'll go ahead and remove those arguments.
These changes are to address the points raised in: ubccr/xdmod-xsede#1154
ryanrath
force-pushed
the
simplify_me_formats
branch
from
f5d9f46 to
30286ad
Compare
3 participants
Description
Refactored
getFormatto the much simpler new functionvalidateFormatand removedgetFormat.Motivation and Context
These changes are to address the points raised in: https://github.com/ubccr/xdmod-xsede/issues/1154
Tests performed
All automated tests.
Checklist: