search

DASSL / Gradebook

Open-source product to provide a practical means for instructors to record student attendance and assessment
Other
8 stars 4 forks source link

fixed the way that the start and end dates for each section is extract… #20

Closed hunterSchloss closed 7 years ago

hunterSchloss commented 7 years ago

fixed the way that the start and end dates for each section is extracted from the staging table

smurthys commented 7 years ago

The rewrite does look easier to read and maintain. I need to analyze it a bit more and am also eager to see other reviewers' thoughts.

afig commented 7 years ago

I agree, the new version is significantly easier to understand and modify. Conceptually, the solution is correct.

However, it seems to be doing more than is necessary. Since we're selecting only one date from the range at once, I do not believe it is necessary to use string_to_array(). If we eliminate that, then that also saves us from having to use array access in the subquery that is inserting into Term.

Another optimization would be to only concatenate the year to the date after the MAX and MIN dates have been found

There also seem to be a couple of syntax issues/places where sDate and eDate have been switched.

However, it is certainly much easier to understand what is going on with the query now than before.

wildtayne commented 7 years ago

This looks pretty good. I wonder if the to_date casts are necessary - I think Postgres will coerce string to date. We may want to keep it for clarity, however in that case it would probably be more clear to do the cast after the concatenation.

wildtayne commented 7 years ago

I think this can be merged once the conflicts are resolved.

smurthys commented 7 years ago

@hunterSchloss please resolve conflicts as soon as practical so these changes can be merged before the major changes in the pipeline complicate conflict resolution.

smurthys commented 7 years ago

@srrollo: In the interest of time, would you please resolve the conflict (seems pretty straightforward) and merge after PR #28 is merged? You will be doing so on behalf of @hunterSchloss.

wildtayne commented 7 years ago

I can resolve the conflicts on this one. I'll merge master and import-openClose-fixes now, which should resolve most of the conflicts. I'll check this branch again once #28 is merged.