-
Notifications
You must be signed in to change notification settings - Fork 8
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
fix: Datasette-serve command not running correctly on Windows #52
Conversation
Hello @Poiuy7312, please note that you need to use the commit standard that we have adopted for this project: https://www.conventionalcommits.org/en/v1.0.0/. |
Hello @Poiuy7312 your description of this PR needs to clearly explain what this will change in the main codebase and in the main tool once it is merged. Can you please furnish an example of what did not work before you made the changes and then what now does work if we merge this PR? You are also welcome to reference an issue in the issue tracker when you explain your PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you've successfully identified and remedied a bug in Chasten and I think this looks good.
I see this code also has a test which would be @tuduun's area of expertise. I'd like to see his review specifically on this PR before approving and merging. Thanks for your work!
Hello @Poiuy7312 is this PR fully updated so that it contains the new build matrix setup for running everything in GitHub Actions? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on configuring the path for all macOS, windows, and Ubuntu. You even tested the code! I will grant the merge request, and I would encourage prof @gkapfham to take a final look at this before anyone merges it!
Yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good.
@tuduun We are still waiting on your review, when you get the chance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An important fix! Passing build, some additional testing to support new code--looks like a winner to me! (Sorry the review took so long!)
This adds a function in util.py called get_OS() which gets what the OS of the user so MacOS, Linux or Windows. This was implemented because there was an issue with the Datasette-serve command on windows would look in the wrong directory for the executable for datasette. So I changed to look in a different directory Scripts if its run on Windows and then it looks for them in the Bin file when run on anything else
I also added a function in database.py which creates a path to the executable which the function Datasette-serve previously did itself. This was done because it made it a lot easier to make sure that the added feature works as intended and allowed me to write a test case for it.