Refactor courses to course#1732
Conversation
Coverage Report for CI Build 4030048Coverage decreased (-0.004%) to 58.608%Details
Uncovered Changes
Coverage Regressions15 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
|
|
||
| import Data.Aeson (FromJSON (parseJSON), ToJSON (toJSON), genericToJSON, withObject, | ||
| (.!=), (.:), (.:?)) | ||
| import Data.Aeson (FromJSON (parseJSON), ToJSON (toJSON), genericToJSON, withObject, (.!=), (.:), |
There was a problem hiding this comment.
I believe this change happened because of the husky pre-commit that occurred when I added my commit. I'm confident this is true because I attempted to make a new commit and push with just this change, but husky's pre-commit (specifically, the stylish-haskell step) told me that lint-staged failed and that it cannot make an empty commit.
I took a look at the stylish-haskell step, where long_list_align is set to inline (so the import tries to fit as many import items on one line as possible). I also noticed that it was previously crammed on one line within the codebase and this change (more imports go on the second line) was only introduced in PR #1720.
There was a problem hiding this comment.
Ah that's interesting, okay you can leave this change as-is 👍
| in | ||
| (Courses code | ||
| (Course code | ||
| (Just title) |
There was a problem hiding this comment.
Modify the rest of the indentation here to keep the arguments to Course aligned
| - Updated documentation in `app/Util/Blaze.hs` | ||
| - Moved the `Course` data type from `Database/Tables.hs` into `Models/Course.hs`, renamed it to `CourseData` | ||
| - Removed `SvgJSON` data type in favour of `([Text], [Shape], [Path])` | ||
| - Refactored the `Courses` table to `Course` with a database migration |
There was a problem hiding this comment.
Do a merge from master and note the updated changelog (from the recent 0.8.0 release); make sure to put this entry under "unreleased")
| import qualified Data.Text as T | ||
| import qualified Data.Text.Lazy.IO as LTIO | ||
| import Database.Tables as Tables | ||
| import Database.Tables (Time (..)) |
There was a problem hiding this comment.
The reason I made this change was because the function addCourseHelper has a courseCode parameter, which overlapped with the courseCode accessor function from Database.Tables. It gave me a name-shadowing warning that didn't occur before because the accessor function was previously coursesCode. The husky pre-commit tests will pass, but the CircleCI tests won't pass.
I came up with 3 options:
- Rename the parameter to be
cCodeinstead ofcourseCodeso that it's different from the accessor function. However, I did notice that the other parameters werecourseSessionandcourseSection, which is why I originally didn't change it. Alternatively, I could change all 3 parameters to becode,session, andsection, orcCode,cSession, andcSection. - Have
import qualified Database.Tables as Tablesby adding thequalifiedkeyword, but that would involve more changes where several functions and datatypes would be relabelled with the prefix. - Add a comment at the top of the file, ignoring the warning. I noticed this is already done in a few other files (ex:
Database.Tables)
Should I continue with the 3rd option (adding a comment at the top of the file to ignore the warning?)
There was a problem hiding this comment.
Thanks @AngelsandDevsLOL, I didn't realize you changed this to avoid a name shadowing issue, but that does make sense. In that case you can leave the change the way you originally had it, including the (Time (..)). I agree that's the most elegant way to solve this.
Proposed Changes
Building off the previous PR #1721 (which renamed the
Coursedata type toCourseDataand moved it intoModels/Course.hs), this PR renames theCoursesdatabase table toCourse. It adds a v3 migration toDatabase/Migrations.hsso the existing database table is renamed. It updates theCoursesdatatype inDatabase/Tables.hsto beCourse, as well as updating all of the accessor functions fromcoursesXtocourseX.With the
Coursename now free from the previous PR, renaming theCoursestable toCoursemakes the schema consistent with the rest of the table names and naming conventions....
Screenshots of your changes (if applicable)
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:
Questions and Comments