Skip to content
This repository has been archived by the owner on Jul 10, 2022. It is now read-only.

add parseSchedules.py (preliminary) #7

Merged
merged 30 commits into from
Jan 21, 2015
Merged

add parseSchedules.py (preliminary) #7

merged 30 commits into from
Jan 21, 2015

Conversation

anbenson
Copy link
Contributor

@anbenson anbenson commented Jan 9, 2015

i haven't updated the readme yet (i'll do that in the next couple days) but this script works decently well. the instructions for how to use it are in the file. let me know about any errors you find, so i can fix them (i know of a couple that i'll be fixing soon).

# Usage: python parseSchedules.py [QUARTER] [OUTFILE]
#
# QUARTER: The school quarter of the schedule desired
# (one of S/M1/M2/F)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is data for Summer semester available?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Summer 1 and Summer 2 are M1 and M2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the two scripts use different terminology for this. Justin's script uses semester while this one uses quarter. I'm not as familiar with the data, but should we pick a name for both of these fields and go with it, or are the names good as they are?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay, I thought those were minis. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I see that Justin's script has U for summer. Does that distinguish between Summer 1 and 2? For my script I have to since they are different links.
I'm ok with "semester" if we want that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My script has U for summer as the course description website doesn't make a distinction between Summer 1 and Summer 2. Perhaps we should have the semesters listed by the course description page associated with the course number, but each lecture/section has their own semester listed too?

I could have U be replaced by M1 and M2, but I'm not sure if the "Summer" listing means it's offered during both quarters.

@jez
Copy link
Contributor

jez commented Jan 10, 2015

@justingallagher @anbenson It'd be awesome if we could get some unit tests for your two scripts.

@jez
Copy link
Contributor

jez commented Jan 10, 2015

The naming conventions between parse_descs.py and parseSchedules.py are way off, even looking at the filenames. Personally, I prefer the lower_snake_case used in parse_descs.py, as it's a little more idiomatic-Python. Could you update yours variable and function names to match Justin's?

If you're looking for a style guide reference, see PEP 8.

sys.exit()

# parse data
data = parseDataForQuarter(sys.argv[1])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put the command line args into descriptive variable names before using them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be fixed

@justingallagher
Copy link
Contributor

I created issues #8 and #9 for unit testing the scripts.

@tomshen
Copy link
Contributor

tomshen commented Jan 10, 2015

Style issues:

@tomshen
Copy link
Contributor

tomshen commented Jan 13, 2015

Any progress?

@jez
Copy link
Contributor

jez commented Jan 20, 2015

Is this ready to be reviewed? I notice that you still have a couple TODOs in your code, but I'm not sure if those are like long-term TODOs or not.

@anbenson
Copy link
Contributor Author

Not quite. I have a few local commits I haven't pushed, and I'm still figuring out how logging works so I can address a few last TODO's.

@jez
Copy link
Contributor

jez commented Jan 20, 2015

Cool. Feel free to keep pushing again this PR, then, until you're ready, and leave a note or Facebook message when it's ready.

@anbenson
Copy link
Contributor Author

All right, I think I've finally finished addressing most of the comments, and fixing the parsing bugs. Please take a look.

### Output format

Beware that any field below may have null or "TBA" instead of a string as a value.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "null"? The string null? The empty string? None?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JavaScript value null. How do you suggest I make that explicit?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrap it in backticks.

@jez
Copy link
Contributor

jez commented Jan 20, 2015

I left two comments, I can't see anything else. I also tested it locally; it seems to work!

Gonna pass this off to @tomshen for further review.

------------|-----------|------------
instructors | [String] | List of last names of instructors of the lecture.
letter | String | The lecture's identifier. Typically a capital letter or something like "Lec 1".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more sense to call this lecture_number? Are there situations where it isn't something like "Lec 1" or "Lec 2"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it's rarely something like "Lec 1" or "Lec 2". It's usually a capital letter (in lectures without recitations). Only awesome classes like 15122 use "Lec 1" or "Lec 2". I'm using "letter" so that it's somewhat consistent with the sections.

@tomshen
Copy link
Contributor

tomshen commented Jan 20, 2015

Once you make fix all the stuff noted in the comments, I think this is good to go.

@anbenson
Copy link
Contributor Author

^ addresses jake's comments

@anbenson
Copy link
Contributor Author

^pull plz?

@jez
Copy link
Contributor

jez commented Jan 21, 2015

:shipit:

jez added a commit that referenced this pull request Jan 21, 2015
@jez jez merged commit 5077468 into ScottyLabs:master Jan 21, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants