AstuteSource / chasten

:dizzy: Chasten Uses XML and XPATH to Check a Python Program's AST for Specified Patterns!
https://pypi.org/project/chasten/
GNU General Public License v2.0
7 stars 8 forks source link

Remove the dependency on pysqlite3 to improve installation on MacOS and Windows #47

Closed boulais01 closed 11 months ago

boulais01 commented 11 months ago

Attempt to make a version of chasten without the pysqlite3 dependency

laurennevill commented 11 months ago

I think this is some really great work @boulais01!! I'd like to see some feedback from other developers in the class who are facing this same issue and are using different operating systems. Once there is a good mix of people agreeing that this solution works as expected, then I think this should be good to merge. I hope this sounds good to everyone?

boulais01 commented 11 months ago

The steps involved in creating this, from Issue #46 : "I removed pysqlite3 from the pyproject.toml file manually, re-generated the poety.lock file, ran poetry install, and then ran poetry run pytest -x -s -vv -n auto, the command intended to run all of the tests. This passed all of the tests. I will be pushing this to the pysqlite3-removal-test-branch shortly; I'd appreciate additional eyes/testing on it."

Poiuy7312 commented 11 months ago

Issue #38 Has some documentation of this

KellerLiptrap commented 11 months ago

I tried this solution on Mac OS and it worked for me.

EvanNelson04 commented 11 months ago

I tried this for Mac OS and it worked perfectly

boulais01 commented 11 months ago

Great; If any Linux users can get this working please let us know, since this was made on Windows

audreyblarr commented 11 months ago

Confirming that this works for Windows, thank you so much for the insight!

simojo commented 11 months ago

The following works on linux :penguin: :heavy_check_mark: running the following:

> poetry lock
> poetry install
> poetry run pytest -x -s -vv -n auto
...
Results (12.79s):
        66 passed
MilesF25 commented 11 months ago

I am using the Windows Linux subsystem and I encountered an error when trying to run the poetry test. "poetry run pytest -x -s -vv -n auto" The said I was missing the numpy module, so I added it to the poetry dependencies. After adding it all test passed. Let me know if there are any issues.

simojo commented 11 months ago

@boulais01 it's worth noting that the package sqlite_utils is used here. I'm not sure if this would be using pysqlite3 in the backend or not. I just want to be safe. None of our test cases use any of the SQL API from what I understand. Someone correct me if I'm wrong, though.

simojo commented 11 months ago

closes #38 closes #39 closes #44 closes #46

boulais01 commented 11 months ago

@simojo If that is a part of pysqlite3, then we can add it as an import in that file, according to some research Audrey has done. I think we're clear though.

boulais01 commented 11 months ago

@bergasanargya Is this acceptable to you, or do we need to add something else for you to approve this?

gkapfham commented 11 months ago

@laurennevill you need to formally use the approve feature in GitHub to sign off on this PR.

laurennevill commented 11 months ago

@laurennevill you need to formally use the approve feature in GitHub to sign off on this PR.

I believe I have already done that.

boulais01 commented 11 months ago

@MilesF25 said that it was necessary to pass the tests in the Windows Linux Subsystem.

bergasanargya commented 11 months ago

If everyone has this working without the pysqlite3 dependency, I approve of this pull request, and agreed to this merge

simojo commented 11 months ago

@Poiuy7312 would you please revert your commit https://github.com/AstuteSource/chasten/pull/47/commits/84714d82fa9e3287979c5228b2c6d25781c854fd so that the build may pass?

boulais01 commented 11 months ago

@Poiuy7312 We need this merged ASAP, did you revert the changes you made?

laurennevill commented 11 months ago

Once the commit by @Poiuy7312 is reverted and the build passes, we can move on with getting this PR approved and merged.

boulais01 commented 11 months ago

@Poiuy7312 The goal is for you to revert your commits, so we return to the commit that added numpy. Can you do this?

Poiuy7312 commented 11 months ago

Yeah I'm working on it

On Fri, Sep 22, 2023 at 12:14 PM boulais01 @.***> wrote:

@Poiuy7312 https://github.com/Poiuy7312 The goal is for you to revert your commits, so we return to the commit that added numpy. Can you do this?

— Reply to this email directly, view it on GitHub https://github.com/AstuteSource/chasten/pull/47#issuecomment-1731684280, or unsubscribe https://github.com/notifications/unsubscribe-auth/A2ZWPMGWEDFIO25TWBR4VYTX3W2MRANCNFSM6AAAAAA4VEY6UE . You are receiving this because you were mentioned.Message ID: @.***>

gkapfham commented 11 months ago

Hello @Poiuy7312 can you please share a status update here?

Poiuy7312 commented 11 months ago

I have reverted all of my changes

laurennevill commented 11 months ago

It looks like this branch still has conflicts that need to be resolved. Has anyone looked into this?

boulais01 commented 11 months ago

@laurennevill Should be clear of conflicts now

boulais01 commented 11 months ago

@gkapfham @laurennevill @bergasanargya Checks are passing again.