Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added skipif for sqlite3 version > 3.24.0 in tests history #11401

Merged
merged 1 commit into from Oct 18, 2018

Conversation

LucianaMarques
Copy link
Contributor

This pull request is an attempt to solve issue 11372.

What I did:

  1. Identified that problem was ocurring in file test_history.py in /tests/ folder insider /core/, in function test_history, this can be checked on the first messege on the issue.
  2. As also said on the issue, problem seems to be happening due to sqlite version is newer than 3.24.0.
  3. So as recommended by Carreau, I inserted a skipif at the top of this function for sqlite's versions newer than 3.24.0

I have no idea how to test this. I'de like some suggestions if possible!

Some observations:

  • My IDE (PyCharm) made some space and line formatting on its own.
  • I don't understand much about sqlite just yet, but this solution to me seems not to solve the problem itself, so I would like some pointers from more experienced developers to think more about it and maybe open a new pull request in the future.

@toddrme2178
Copy link
Contributor

It doesn't work for me. sqlite3.version_info gives (2, 6, 0). I think you might want sqlite_version_info, which gives (3, 25, 0) for me.

@LucianaMarques
Copy link
Contributor Author

Hi,

I just did the alterations @toddrme2178 commented.

I'm having problems with squashing all commits into one, but my changes are there.

@Carreau
Copy link
Member

Carreau commented Oct 16, 2018

I'm having problems with squashing all commits into one, but my changes are there.

Do you mind if I do that for you and push on your branch ?


Also side-note, you want to learn how to work on multiple branches, if you look at the top of this PR you can see that the request is to merge LucianaMarques:master into ipython:master, that prevents you from working on multiple things at the same time.

Usually you want to something along the line of:

$ git branch fix-11401
$ git checkout fix-11401
... work
$ git push [...]

And the the PR will ask to merge LucianaMarques:fix-11401 into ipython:master
And you can keep working on other branches, without worry of fix-11401 being modified.

The workflow here is approximatif, you are free to modify it, but learning on how to manage multiple branches will be extremely helpfull

Do not worry git is not obvious, but you'll learn your way around it.

@Carreau Carreau merged commit 1a33fa3 into ipython:master Oct 18, 2018
@Carreau Carreau added this to the 7.1 milestone Oct 18, 2018
@LucianaMarques
Copy link
Contributor Author

Hi @Carreau ! Thank you for the help with squashing the commits. I'm sorry for not answering you here earlier, I was trying to fix it for myself. If you can, please let me know how you did it, I'm not used just yet with the HEAD file.

And thank you very much for merging my pull request! I'm still looking for simple issues to fix (I'm learning a lot from collaborating), if you find something please let me know. Thanks!

@Carreau
Copy link
Member

Carreau commented Oct 18, 2018

I'm sorry for not answering you here earlier, I was trying to fix it for myself.

No worries, I had an hour or two where I could work on that, so I did.

Here is a gif that re-does the rebase/squash

rebase example

The I pushed with the --force-with-lease option on your branch.
I think you already found a couple of other issues to work on. I'll try to keep tagging easy issues.

You're doing good so far.

ellert pushed a commit to ellert/ipython that referenced this pull request Oct 30, 2018
@Carreau Carreau modified the milestones: 7.1, 5.9 Nov 1, 2018
Carreau pushed a commit to ellert/ipython that referenced this pull request Nov 3, 2018
Carreau added a commit that referenced this pull request Nov 3, 2018
Backport PR #11401 on branch 5.x (Added skipif for sqlite3 version > 3.24.0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants