Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: PortalFetch #20

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

refactor: PortalFetch #20

wants to merge 5 commits into from

Conversation

uniquePaul
Copy link

crawler_download.py:
can accept multiple quarter values now

crawler_download.py can accept multiple quarterValues now
if school == "De Anza":
school = "De_Anza"
fileNameOutput = year + "_" + quarter + "_" + school + "_courseData.json"
n = 6

Choose a reason for hiding this comment

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

avoid naming "n". the naming should be meaningful here. moreover, if a number is a constant, please use upper case. reference:https://www.python.org/dev/peps/pep-0008/#constants

PortalFetch/crawler_download.py Outdated Show resolved Hide resolved
@yifeili98
Copy link
Member

A friendly suggestion:
When you write commit message for refactor, try to summarize which section you are refactoring, but not the entire project. For example, the commit message for this one could be refactor: enable fetching data from multiple quarters

Copy link
Member

@yifeili98 yifeili98 left a comment

Choose a reason for hiding this comment

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

Please also update README so we know the updated usage of config

Copy link
Member

@yifeili98 yifeili98 left a comment

Choose a reason for hiding this comment

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

Have you tested the code locally? The logic does not seem correct, you use quarter_downlist.select_by_value(value) at line 283 where value is a string of multiple quarter values, webdriver will not be able to find this string and will crash when multiple quarter values are inputted.

@yifeili98 yifeili98 requested a review from hk-mp5a3 July 18, 2021 21:36
Copy link
Member

@yifeili98 yifeili98 left a comment

Choose a reason for hiding this comment

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

Also, please remember to modify README since you are changing the config format.

Comment on lines 230 to 232
school = schoolSwitcher.get(quarterValue[5], "")
quarter = quarterSwitcher.get(quarterValue[4], "")
if quarter == "Summer":
Copy link
Member

Choose a reason for hiding this comment

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

Please remember to add the comment back...

list_group_item = None
while web_driver_counter:
quartervalue = parser.get('config', 'quarter_value')
quartervalueList = quartervalue.split('_')
Copy link
Member

Choose a reason for hiding this comment

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

It is better to use ',' to represent a list of values, it should be a quick fix since all other functionalities are working, please remember to trim after split(',') since users could add spaces after comma

login_myportal(driver)
# Wait for the 'list-group-item' can be found and clicked
web_driver_counter = 400
list_group_item = None
Copy link
Member

Choose a reason for hiding this comment

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

Actually you don't need this line of code

Copy link
Member

Choose a reason for hiding this comment

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

I mean list_group_item = None

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants