search

civisanalytics / civis-python

Civis API Python Client
BSD 3-Clause "New" or "Revised" License
34 stars 26 forks source link

Update pyyaml version #293

Closed keithing closed 5 years ago

keithing commented 5 years ago

From what I can tell the change from V3 to v5 involves the infamous safe_load debate of pyyaml, but not breaking changes that affect us. load is now deprecated. The only place we're using pyyaml is for the CLI.

Resolves #292

keithing commented 5 years ago

Alternatively, we could remove the maximum version for all install_requires, which would be a more permanent fix. Any thoughts on that @mheilman ?

mheilman commented 5 years ago

I can see the pros/cons of having that maximum version there or not, and I don't have a strong opinion. Given that the behavior of pyyaml.safe_load is unlikely to be subject to breaking changes even in major versions, I'd be fine with not having the < restriction in setup.py

keithing commented 5 years ago

@mheilman, how about we leave the < in for now, and then we decide whether to refactor the install_requires more generally at a later date? I think we're being a little too prescriptive in our install_requires. According to the docs, we should be doing the pinning in our requirements.txt but not necessarily in install_requires.

mheilman commented 5 years ago

OK. What you have seems fine to me, but there's a conflict that needs to be resolved.

The setup.py file is fairly specific, but I don't think it's too far off from what the python docs recommend. We could remove the upper bounds on major versions, but that has pros and cons.

We should probably pin to very specific versions in requirements.txt.

keithing commented 5 years ago

Merge conflict has been fixed!