Hack

[ZackBotkin]

This branch doesn't really remove anything from what you've done, but kind of reorganizes it into classes. I didn't test any of my stuff beyond connecting to a local sqlite database, so I at least know that works.

If you want to run it, you'll want to do

PYTHONPATH=. python simqle/provider.py

Take a look, see if you like it.

To be honest, I'm not sure why the README commit got sucked in to this pull request.

[codecov-io]

Codecov Report

Merging #33 into dev will decrease coverage by 10.18%. The diff coverage is 89.52%.

@@            Coverage Diff            @@
##            dev      #33       +/-   ##
=========================================
- Coverage   100%   89.81%   -10.19%     
=========================================
  Files         5        4        -1     
  Lines        72      108       +36     
=========================================
+ Hits         72       97       +25     
- Misses        0       11       +11

Impacted Files	Coverage Δ
simqle/constants.py	100% <100%> (ø)	:arrow_up:
simqle/__init__.py	100% <100%> (ø)	:arrow_up:
simqle/connection_manager.py	88.76% <88.76%> (ø)
simqle/internal.py	92.3% <92.3%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8e5a380...6056922. Read the comment docs.

[TomMalkin]

Ah I see what you're going for here - yeah I like it a lot, definitely a better way.

I won't have access to a PC for the next 3 weeks unfortunately so I won't be able to pay around with it until then.

Do you mind changing the target branch to dev rather than master? I suppose I should add contribution guidelines.

I like the provider class because it would be package specific in the case that an imported module also uses simqle, so that is really nice. It's a shame it makes it slightly more verbose though.

I'll have a play around as soon as I can. Looking good!

[ZackBotkin]

You got it

[ZackBotkin]

Hey, I'm gonna try and get some work done, assuming I have time. A few questions

1) I'm going to replace the original code with the classes, if that's all right with you. We don't need to have both bc the functionality is now duplicated 2) You want me to target my pull requests onto dev branch? no prob. In that case, I'll probably just kill the existing pull request and re-make another. 3) Once I get the changes in, how do we go about merging them in, so that I can take another issue or two? It would probably be best to get that refactoring part of master before writing more code based off of the old code.

[TomMalkin]

Awesome! I wish I had access to a PC so I could dive in and help. Here are my thoughts re the refactor.

I was planning to have the master branch always live to pypi so that was why I asked for the pr to go into Dev instead. I don't have pypi upload automated yet as part of the Travis ci build.

With the refactor, I was thinking that a provider class instance could be created by using the load_connections function that is stored at the simqle module level (similar to the CONNS dict), and a recordset and execute_sql function could be wrappers for calling those same functions in that provider instance. In other words a user could use either their own provider instance, or the module level one. Having access to that would be handy for a number of reasons but the main advantage is that existing code would still work after the refactor.

Before the refactor can be merged in to master, it needs to pass all integration tests, and the main functions should have unit tests (I'm happy to write any tests). Also, could you please make sure the code style is the same as the current style (try running pylama over the code, I usually try to stick to their style). Also things like if name == main functions should be removed (until we add cli later!)

If you fork from your refactor that should be fine, I won't be adding to the code in the short term so there shouldn't be any merge conflicts.

Let me know if you agree with that instance level idea - I think keeping the code as simple as possible from an end user's perspective should be our no. 1 goal.

[ZackBotkin]

I haven't had time recently, might have a bit coming up this weekend.

List for myself 1) Clean up the refactor, get it passing the tests, make sure style is consistent, remove main, etc 2) Kill the existing pull request, remake with the target of dev branc 3) Fork off of the refactor branch

[TomMalkin]

Hey! Absolutely no worries at all - I'm MIA myself for at least 1.5 weeks so no hurry whatsover.

[ZackBotkin]

How do you typically run the integration/unit tests?

[TomMalkin]

The unit tests can be run locally with just pytest, but the integration tests are through Travis CI because I make use of their easy database services (see the Travis yaml file for details). These should run on every commit to your branch, with the results of the tests, plus coverage, should appear on this PR

[ZackBotkin]

Hey, sorry just seeing this. I started writing some integration tests on sunday, using the "behave" framework.. I'm wondering if I can hook them into travis. To be honest, I've never heard of Travis and I don't really know how to use it, going to read up a bit to see if what I'm thinking of is possible.

EDIT : It looks like your travis file is simply calling a command... we could almost certainly do something like this

install:
  - pip install codecov pytest
  - pip install behave

script:
  - coverage run -m pytest ./tests/unit-tests/
  - PYTHONPATH=. behave tests/integration-tests/behave-tests/

I recommend you check out behave https://behave.readthedocs.io/en/latest/

Super easy to add new tests using english-like syntax

[TomMalkin]

Very cool! I've never heard of behave, but it looks handy. I might split out the issues so one is the refactor, and one is the integration tests for the new code. If the refactor is done well as per above, it should actually work with the current integration tests.

[ZackBotkin]

Sounds good.

I've been away from internet so I havent been able to push, but I have a full end to end sqlite integration test where it creates an sqlite .db file, rcreates a table, reads a conf, inserts to table, confirms insertion.

Working on mysql next, and will push "hack-hack" branch when I can. Roughly speaking,

Hack branch -- refactoring Hack-hack branch -- integration tests/behave

Maybe a tiny bit of overlap.

[TomMalkin]

Alrighty - quick question, thoughts on removing the provider class and moving the recordset query to the ConnectionManager class? It seems like a thing wrapper to the connection manager and not sure what it's purpose is

[TomMalkin]

Ok I didn't realise you could do this but apparently I pushed to your hack branch - perhaps I have push privilege because it's part of the PR? I'm not sure. Anyway, sorry if that has screwed anything up for you!!

I've implemented your refactor with a few changes. Best thing is that by using an internal connection manager that is controlled by functions of the same name as the original (as I mentioned somewhere), a) all existing projects will work after the refactor upgrade with no code change required, and b) all integration tests as they stand work without any changes (for the same reason). Check out internal.py for that implementation. Handy!

Excuse the million commits - we'll squash those before going into master to clean it all up.

Some changes I made were:

• Cleaned up .gitignore - this should be kept clean, and test artifacts shouldn't be left in the project directory anyway. Also, as I found out recently, you can have your own hidden .gitignore inside the .git folder (I forget the details) which is good for collaboration where you don't want to change other people's .gitignore.

• Removed unit tests from .travis for the time being as discussed before

• Removed the Provider class, and implemented it in ConnectionManager

• Cleaned up the ConnectionManager and Connection Classes (now should follow pep8). Connection was renamed to _Connection, because we don't want users to import and use that class. Quite a lot changed here, probably best you cast your eye over connection_manager.py to get the details. Let me know if you have any thoughts on those ones.

  • added a default connection files constant - more on that later, that will be part of another PR discussion

  • internal.py was added - this is basically just for backwards compatibility with current projects as mentioned above

  • cleaned up the mysql integration test (you accidentally added a sqlite connection to that .connection.yaml file)

  • Added a whole bunch of TODOs in the code as I thought of things - will turn those into Issues at some point

So after all this, the only things we need to do to get this PR going is:

• Update the README (explaining the new usage possibilities from your refactor)
• get the code cov up to 100% (some items aren't covered at the moment but that's all g)
• remove the old files once we're happy with the new ones (from my old code - functions.py, sql_tools.py etc)

Once this is done, we can start a conversation about upgrading to Behave etc, which is pretty exciting!

Anyway, let me know your thoughts!

[TomMalkin]

I've added the SIMQLE_TEST switch as per the README. Also, there is a spot there to add your details as a Contributor if you like, which will be added when this PR is merged

[TomMalkin]

I can add the extra integration tests on this branch if you like if you want to update that contributor section of the README. There is actually not much more to do for this PR now and getting it uploaded to pypi and into master should be a goal I think, and then we can use the other PR to really get Behave going nicely

[ZackBotkin]

Hey, sorry it's been a while. Busy week.

I'm on board with removing the provider class. I wanted to have it initially because it could be an abstraction surrounding the data. If you are dealing with the Provider, you don't have to care about any connection logic, just the SQL that you want to execute. The connection manager is the thing that handles the connection logic. If you feel it's best though, I'm fine removing it.

So, since you were able to push to my branch "hack" then it sounds like I will need to rebase on top of that with my behave branch "hack-hack", to capture the newly pushed changes in "hack"... that's fine, although I'm not currently sure how to pull in your changes into my local fork of the project. Next time, I'm going to use more descriptive branch names :)

One last point, is it fine if we hold off on making the tests work for this pull request? It seems like whatever tests we end up writing to get things up to 100% will immediately get replaced with their "behave" versions found in the second pull request. Is it worth it to expend effort into making the travis build/code coverage happy, just to immediately kill off those tests when we pull in hack-hack? I would think not.

[TomMalkin]

Good point about those tests. The main point is that by writing the internal.py stuff, the existing tests all pass as per normal, (sqlite, mysql, and postgresql all work fine). I haven't changed or added anything to the existing tests.

If you add your contribution section to the README I'll clean up the files that are no longer needed and we can get this PR merged into dev. We can then focus on the other PR going into dev as well, and once those behave tests are looking good and are at 100% for sqlite, mysql and postgresql in travis, we'll merge it all into master and get the version updated.

[TomMalkin]

All merged! Ok now we just need to get Behave working for the different databases and get coverage back to 100% on the other PR. We'll continue the discussion there