Refactor JSON parsing to replace data types with helper functions - #1789
Refactor JSON parsing to replace data types with helper functions#1789AngelsandDevsLOL wants to merge 1 commit into
Conversation
Coverage Report for CI Build 0Coverage remained the same at 58.192%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
david-yz-liu
left a comment
david-yz-liu
left a comment
There was a problem hiding this comment.
@AngelsandDevsLOL nice work! I left a few minor comments, but please also add new tests for the insertCourses function (this file currently isn't being tested!).
| -- | Helper function to flatten the list of DB Objects | ||
| flattenDBList :: DBList -> [MeetTime] | ||
| flattenDBList (DBList meetings) = concatMap (\(DB meetTimes) -> meetTimes) meetings | ||
| forM_ (parseMeetingInfo respBody) (mapM_ insertMeeting) |
There was a problem hiding this comment.
Since parseMeetingInfo returns a Maybe [MeetTime], you can simplify to use just a bit of pattern-matching here and display an error message when the parsing fails. (You also don't need the do, which is for multi-step monadic computations.)
case parseMeetingInfo respBody of
Nothing -> -- print an error message
Just meetTimes -> ...| json <- decode respBody | ||
| flip parseMaybe json $ \obj -> do | ||
| maybePayload <- obj .:? "payload" | ||
| case maybePayload of |
There was a problem hiding this comment.
I don't think you need a case here. Instead use .: above, and if it fails then Nothing will be returned, which I think is okay.
| dbList <- mapM parseJSON courses | ||
| return $ DBList dbList | ||
| Nothing -> return $ DBList [] | ||
| courses :: [Value] <- pageableCourse .: "courses" |
There was a problem hiding this comment.
Is this type annotation necessary? I would think that GHC can infer the type [Value] based on how courses is being used below.
There was a problem hiding this comment.
It is necessary, because concat is defined as concat :: Foldable f => f [a] -> [a], where courses should be [Value], but the [] is not inferred. The same thing happens with mapM, where it takes mapM :: (Traversable t, Monad m) => (a -> m b) -> t a -> m (t b), where courses should match t a but doesn't know what t (in this case []) is.
If it isn't included, then courses gets type Any Value, and GHC tells us that courses should be courses :: t0 Value, but t0 is ambiguous.
3 participants
Proposed Changes
This PR refactors
app/WebParsing/UtsgJsonParser.hsto remove theDBandDBListdatatypes in favour of two helper functions,parseMeetingInfoandparseCourse.The
DBandDBListdatatypes exist to carryFromJSONinstances for parsing the timetable API response. They aren't used anywhere else in the codebase and are constructed at a single call site only to be unwrapped again immediately by a helper function. Replacing them with helper functions gives the same behaviour and makes the parsing logic more readable.Type of Change
(Write an
Xor a brief description next to the type or types that best describe your changes.)Checklist
(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the
[ ]into a[x]in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)Before opening your pull request:
After opening your pull request: