pittcsc / PittAPI

An API to easily get data from the University of Pittsburgh
https://pittapi.pittcsc.org
GNU General Public License v2.0
105 stars 30 forks source link

Refactoring Textbook API #68

Closed azharichenko closed 6 years ago

azharichenko commented 7 years ago

Objective

The goal of this PR is to increase the testability of the code, improve performance by taking advantage of features of the language and having consistent documentation throughout the code. Also, improving requests to the textbook website to cut down parsing through data.

Textbook API Changes

RitwikGupta commented 7 years ago

Is this still an active WIP?

azharichenko commented 6 years ago

Yes, this is still a work in progress. I just haven't had time for this project recently because I'm learning other things like machine learning, python, flask, etc.

azharichenko commented 6 years ago

I believe at this point this PR is ready for to be reviewed.

azharichenko commented 6 years ago

@dzheng256 Just fixed the typo. And with the getting rid of _get_department_number I had this interesting alternative idea that I'm working on with having a dictionary with all the departments. Since both textbook and course API have common departments, why not have a dictionary in __init__.py that gives access to the departments.

class DepartmentCodes(dict):
     def __init__(self):
     with open(os.path.join(__file__, '..', 'department.txt')) as file:
         text = ''.join(file.readlines())
         dict.__init__(self, json.loads(text))
      
     @property
     def codes(self):
        return self.keys()

departments = DepartmentCodes()

This is sort of what I came up with, for now, essentially having a text file with JSON that shows various details about each department and just quickly parsing through it when the library is loaded. The simple DepartmentCodes object is a dictionary with other properties such as being able to get all the codes by saying department.codes and being able to lookup the correct query or department number value very easily.

{
    "ADMJ": {
        "subject": null,
        "value": "22399"
    },
    "ADMPS": {
        "subject": "core",
        "value": "22400"
    },
    "AFRCNA": {
        "subject": "core",
        "value": "22401"
    },
    "AFROTC": {
        "subject": null,
        "value": "22402"
    },
    "ANTH": {
        "subject": "core",
        "value": "22403"
    },
    ...,
}
dzheng256 commented 6 years ago

@azharichenko Yeah that's a good idea.But why not just make a json instead of a txt to load?

azharichenko commented 6 years ago

It loads JSON from a text file.

dzheng256 commented 6 years ago

But you are using json.loads() and forming a string. You could just make departments.json and do json.load(file) and skip the string formation is what I mean.

azharichenko commented 6 years ago

json.loads() takes in a JSON string and converts into a python dict hence why it's in the __init__ . There is no way to skip getting the string out of the text file.

dzheng256 commented 6 years ago

Okay I don't think I'm explaining myself clearly. What I'm trying to say is you're trying to store JSON in a .txt when it should be a .json file which you can load without making a string. Your snippet does something like this:

 with open('departments.txt') as file:
         text = ''.join(file.readlines())
         dict.__init__(self, json.loads(text))

I'm suggesting you do this:

 with open('departments.json') as file:
         dict.__init__(self, json.load(file))
RitwikGupta commented 6 years ago

Dan's suggestion should work regardless of file extension. For semantics, naming it departments.json will be cleaner, but not necessary.

azharichenko commented 6 years ago

Didn't realize json.load actually can take in the file so I don't have to do the read line nonsense

RitwikGupta commented 6 years ago

@azharichenko Push a commit to fix that, and I'll review

azharichenko commented 6 years ago

I prefer the current format of the code. It is much more readable and we should be using the keyword is since it's a compare operator, as in we are comparing section and None.