GothenburgBitFactory / bugwarrior

Pull github, bitbucket, and trac issues into taskwarrior
http://pypi.python.org/pypi/bugwarrior
GNU General Public License v3.0
740 stars 209 forks source link

Redmine query parameters #886

Closed CaptainQuirk closed 2 years ago

CaptainQuirk commented 2 years ago

:dart: Goals

:panda_face: Implementation

:notebook_with_decorative_cover: Documentation

CaptainQuirk commented 2 years ago

I also saw that a suite of github actions failed on my branch : https://github.com/ralphbean/bugwarrior/actions/runs/2031120214. I used pytest locally and got a 0 exit code (and a bunch of warnings about distutils version classes being deprecated, due to the fact that python is installed with homebrew on my Linux Mint)

ryneeverett commented 2 years ago

It's not clear to me that there is any need for an if/else clause here. Couldn't we continue passing the limit and assigned_to_id parameters in args even with additional query parameters in the url?

CaptainQuirk commented 2 years ago

How should I handle the query setting which is a partial query string, then ? Due to its partial nature, I'm not sure I could parse it into a dict easily.

CaptainQuirk commented 2 years ago

Maybe your thought was to both append the string value of the query setting to the url variable while also filling the arg dict ?

ryneeverett commented 2 years ago

Maybe your thought was to both append the string value of the query setting to the url variable while also filling the arg dict ?

Yes, that was my thought and I think it's the best route unless there is some reason it doesn't work.

How should I handle the query setting which is a partial query string, then ? Due to its partial nature, I'm not sure I could parse it into a dict easily.

I actually think it is fairly easy to parse a query string into a dict with urlparse.parse_qs.

However, if for some reason we're forced to choose between strings and dicts, I think it would be more reliable to serialize the limit and and assigned_to_id like you did than to parse the user's query string which is just another opportunity for error. However, I'd want to serialize it every time, not just when query != '', so we still wouldn't need the else clause. We want to minimize logical branching. Make sense?

CaptainQuirk commented 2 years ago

I actually think it is fairly easy to parse a query string into a dict with urlparse.parse_qs.

A full blown query string, maybe, but something like project_id=10&status_id=15 with no leading & or ?, I'm not sure.

So to wrap it up :

That should indeed reduce the branching !

ryneeverett commented 2 years ago

I think we could completely remove the branching by appending query even if it is an empty string because I believe a trailing ampersand is acceptable.

CaptainQuirk commented 2 years ago

That should do it now !