search

Doveps / mono

Code for Doveps projects
http://doveps.com
MIT License
0 stars 0 forks source link

Refactor query #86

Closed JosiahRegencia closed 6 years ago

JosiahRegencia commented 6 years ago

Was able to remove set_engine_name() and was able to avoid calling it every time I call a function for a set of queries.

One drawback however is: psycopg2 cursor and connections don't close. Because if I close them, we can't be able to run other queries. Unless if we create new ones. Which somehow defeats the purpose of removing set_engine_name(). What happens is that when a query is run it continues to pile up the cache since only one cursor is used. I want to find a way to be able to close a cursor after a certain time it's not used or maybe only after a certain time and then create a new cursor with the same variables. haven't found anything really helpful yet, though.

greenmoss commented 6 years ago

Which issue is this linked to? Do you want me to merge it?

What do you think about the codeclimate issues?

JosiahRegencia commented 6 years ago

85

greenmoss commented 6 years ago

Linking to #85

JosiahRegencia commented 6 years ago

Hi Kurt (@greenmoss ),

Sorry I just checked codeclimate right now. Most of its issues were similar to the issues we previously had which you guys decided to make codeclimate ignore. The issues were that there are some functions that look the same because they seem to have the same structure. The thing is that each function calls a different function in postgres. But its structure is the same so codeclimate has issues with it. But as I said, in the previous this was ignored. I can try to find a way to minimize the repeat of the structure of those functions if you want me to.

JosiahRegencia commented 6 years ago

Like this one:

https://github.com/Doveps/mono/blob/dd024f1e7849049723699822fd7b2427d4b63afe/savant/app/query.py#L59-L71

greenmoss commented 6 years ago

Adding do not merge label, until @JosiahRegencia wants it merged

zerstoeren commented 6 years ago

Is there a benefit to storing the psycopg2 output into a dict to be able to close the connection and open a new one or will that not work? I did this once, but the stored information was not anywhere near what this is doing.

On Feb 3, 2018 5:07 AM, "Josiah Eleazar T. Regencia" < notifications@github.com> wrote:

Was able to remove set_engine_name() and was able to avoid calling it every time I call a function for a set of queries.

One drawback however is: psycopg2 cursor and connections don't close. Because if I close them, we can't be able to run other queries. Unless if we create new ones. Which somehow defeats the purpose of removing set_engine_name(). What happens is that when a query is run it continues to pile up the cache since only one cursor is used. I want to find a way to be able to close a cursor after a certain time it's not used or maybe only after a certain time and then create a new cursor with the same variables. haven't found anything really helpful yet, though.

You can view, comment on, or merge this pull request online at:

https://github.com/Doveps/mono/pull/86 Commit Summary

  • Refactoring query.py
  • Different instance names for flavor and records
  • Removed closer of psycopg2
  • Merge branch 'master' of https://github.com/Doveps/mono into refactor-query
  • Working code. But bad practice
  • query edits applied to unittests
  • Commented close codes
  • Removed comments and print codes
  • remove more comments

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Doveps/mono/pull/86, or mute the thread https://github.com/notifications/unsubscribe-auth/AGbogqAdknPShatbi1w8Mr9ADAqcQJ_-ks5tRC_jgaJpZM4R4Hgi .

JosiahRegencia commented 6 years ago

@zerstoeren the purpose of the json file where the result was stored was for when the new data will be added to knowledge tables.

JosiahRegencia commented 6 years ago

I'm not really sure what benefit you are referring to @zerstoeren

JosiahRegencia commented 6 years ago

@greenmoss can we have this one merged? And then I'll work on the concern of @zerstoeren on a separate ticket? I think I need the changes a I made from this branch. I plan on creating a new branch for the travis-docker stuff. But not have travis-docker deleted yet.

greenmoss commented 6 years ago

Ignoring codeclimate for now. We will eventually want to either customize/ignore these warnings, or fix them.