Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 slot and professors spiders in light of sigarra schedule html page changes #112
Refactor slot and professors spiders in light of sigarra schedule html page changes #112
Changes from 1 commit
43f3d3c
b343501
0bf499b
26ed53c
e3a60f1
50808f8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
None
a possible value? What happens in these cases?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None
was a possible value in the first version of theget_professor_info
method at the time that comment was written. Now that you pointed it out, I removed that outdated comment (# (sigarra_id | None, teacher_name)
)In the current version,
None
is not a possible value. When theteacher["name"]
does not respect the format<professor_id> - <professor_name>
, instead of retrieving theprofessor_id
on the left side of the-
, it justs uses theteacher["sigarra_id"]
number from the values returned from sigarra api.To clarify, the
teacher["sigarra_id"]
field is not really the id that appears on the link of the page of the teacher, which is extremely weird but not up to us since we didn't developed that API.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the
professor_id
is used in the link, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I just see that there is also a
sigarra_url
, which redirect to teacher profile. Can we save that? I think it could be usefullThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the
professor_id
is used in the link.We can save that, good point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the URL is safest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure I understood what you suggested
Do you mean retrieving the
professor_id
from thesigarra_url
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, save the
sigarra_url
in the database as, for example,teacher_link
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that maybe it is probably best to do it in a separate issue from this PR, as it is a lower priority issue and this PR should be merged as soon as possible.