search

bbookman / SkillDemand

Apache License 2.0
1 stars 1 forks source link

Feedback so far #31

Closed santiagobasulto closed 5 years ago

santiagobasulto commented 5 years ago

Hello Bruce and Nirmal! Your project is looking very good. Here's my feedback for what you have so far.

1) Move milestone 2 to a closer date

Milestone 2 has one of the most important tasks which is the indeed scrapper. I remember you told me you had that working. Either case, March 15th seems late for such an important piece. My recommendation is to split Milestone 2 into 2 different milestones, 2a being the scrapper, and 2b being the notebook. The sooner you can get the scrapper done for our review, the better. Please let me know if this is possible.

2) Specify a better form of "deliverable"

The way this will work is with a command line script, right? Can you please provide a couple of examples in your scope document with usage? Just for clarity now.

3) Minimum comments about code

I don't want to do a full code review yet, until the milestone date. But so far it's looking good. I'd advice for better organization, and make main.py a truly main file, only with a copule of lines: it should import all the functions and run them. But again, looks good so far.

4) Other minor issues:
bbookman commented 5 years ago

Thank you for the feedback. I have updated the scope document to help provide more clarity.

I was able to deliver to Nirmal a data dictionary / data model so that he can start working on graphs without real data from the app. So we can deliver on our revised milestone.

The app "works" in that all the scraping is functioning and I can populate a skill list with counts from 3 different job websites. I'm debugging some small issues with getting all the data in the form it should be in.

Important regarding design: I had a half dozen functions doing pieces of the puzzle. However, I ran into an issue where Selenium would throw "StaleElementException". After some digging and some experimentation, the only way to address this that worked was to "do everything in one ugly function". Hence get_skills is bloated as anything and in a perfect world would not look anything like what it does.

There are probably very small pieces of get_skills that I could carve off. I consider that a P2, where the P1 is ensuring the data is exactly how it needs to look for Nirmal to graph

santiagobasulto commented 5 years ago

👍Great! Seems like you're both very far ahead. Scope looks good. I do think it should be better to have some sort of parametrization for the command line program. Maybe it's not individual commands, but being able to specify a file to read from. That can be actually for another milestone, no need to waste time on it.