Python translation #61

emilybache · 2023-03-03T12:41:58Z

The translation is based on the typescript version. The github action runs the tests.

nitsanavni · 2023-03-04T11:50:14Z

.github/workflows/python-build.yml

+on:
+  push:
+    paths:
+      - 'python/**'


nitsanavni · 2023-03-04T11:50:40Z

.github/workflows/python-build.yml

+    - name: Checkout Repository
+      uses: actions/checkout@v2
+
+    - name: Start MYSQL and import DB


consider using https://github.com/marketplace/actions/actions-setup-mysql

This part is just copied from the typescript version. Should probably update all the versions together?

I would leave that for a separate PR

I've found a cleaner way to do that:

https://github.com/nitsanavni/Refactoring-Kata-Lift-Pass-Pricing/pull/1/files#diff-88dc73ea041a254f6d251b3eb8872104b6926163e8a022e18a9b7d6939a99e73R23

nitsanavni · 2023-03-04T11:52:27Z

python/test/test_prices.py

+        (15, 35, datetime.fromisoformat('2019-02-22')),
+        (15, 35, datetime.fromisoformat('2019-02-25')), # monday, holiday
+        (15, 23, datetime.fromisoformat('2019-03-11')), # monday
+        (65, 18, datetime.fromisoformat('2019-03-11')),  # monday


did you use a coverage / mutation tool?

no I just translated the typescript tests.

nitsanavni · 2023-03-04T11:53:32Z

.github/workflows/python-build.yml

+        sudo systemctl start mysql
+        mysqladmin --user=${{ env.DB_USER }} --password=${{ env.DB_OLD_PASSWORD }} version
+        mysqladmin --user=${{ env.DB_USER }} --password=${{ env.DB_OLD_PASSWORD }} password ${{ env.DB_PASSWORD }}
+        mysql -u${{ env.DB_USER }} -p${{ env.DB_PASSWORD }} < ${GITHUB_WORKSPACE}/database/initDatabase.sql  


like 👍🏼

nitsanavni · 2023-03-04T12:07:56Z

.github/workflows/python-build.yml

+        mysqladmin --user=${{ env.DB_USER }} --password=${{ env.DB_OLD_PASSWORD }} password ${{ env.DB_PASSWORD }}
+        mysql -u${{ env.DB_USER }} -p${{ env.DB_PASSWORD }} < ${GITHUB_WORKSPACE}/database/initDatabase.sql  
+
+    - name: Install MySQL odbc driver


🤔 other translations don't rely on odbc, is it a must?

I've experimented with this alternative:
emilybache#1

I used odbc because it makes it easier to switch to a different database. It's probably not necessary here though as you point out

codecop · 2023-03-04T12:25:42Z

Emily, thank you for the translation. Looks very good. There is a small inconsistency to the other projects - because also typescript version has it.

There is a main entry point (class, whatever) which calls createApp and then gets the connection and the application. It will print a message with a test URL and proper port (look into index.ts in the Typescript) and then starts the app by listening. So please instead of having the run of app in the main method of Prices, please create a separate file.
The createApp is also returning the connection, so tests can close it after running. I do not see the tests closing the connection, maybe a method provided to shutdown of process? I think it's OK to get the values app and importing by running the module like it is now, connection is just available globally then?

codecop · 2023-03-04T12:29:35Z

I am missing the Readme that all other languages have. That should be little work, just copy any Readme and change links and commands to run the tests and application.

nitsanavni · 2023-03-04T12:58:36Z

I do not see the tests closing the connection

that's because the test is spawning the app as a child process, and once the child process is terminated it stops connections (at least, I believe it does)

martinsson · 2023-03-05T23:25:56Z

Thanks @emilybache !

I tried to run this version on a macbook

The slight problem is I need to adapt the install of mysql-odbc. I think @nitsanavni suggestion solves that problem. Probably also for windows. However it's not complete (still some references to pyodbc).

I'd suggest

add the readme with the setup suggested by @codecop, I did the following (based on your github workflow)

python3 -m venv venv
. venv/bin/activate
pip3 install -r requirements.txt
PYTHONPATH=src python3 -m pytest

merge as-is noting that the setup is for linux

As a second step finalize @nitsanavni's contribution to make it platform independent.

emilybache · 2023-03-06T07:07:53Z

Thankyou for all your comments! I feel a bit embarrassed I forgot the readme file, sorry about that. I'll fix that and look at your other comments soon.

codecop · 2023-03-06T11:45:37Z

No I feel embarrassed to ask you for changes.

emilybache · 2023-03-09T12:02:46Z

To answer the points from @codecop

I wasn't able to work out how to put the Flask application creation in a different file from the methods with the app.route decoration. It might be possible but I don't know how. Sorry!
The db connection is not being explicitly closed, but it should close automatically when you kill the process. I don't think it's important to do explicitly. It is important that it's created in the same thread where it's used, and i fixed that at least.
I added a README which I hope does the job.

martinsson · 2023-03-12T22:53:57Z

OK Great @emilybache ! this works on a macbook too using the normal dependency of docker.

Nice try with sql lite as well although it fails at ON DUPLICATE KEY UPDATE cost = ?. I don't know how to solve this, other than going all the way to an ORM like sqlalchemy. This was actually the approach taken by https://github.com/martinsson/Refactoring-Kata-Lift-Pass-Pricing/pull/58/files. A MR I never got around to merge. I don't know what to do with it now though. It could be merged as a way of having sqllite. But If anyone has another idea of how to add the sqllite functionality to this project that'd be preferable.

Out of the suggestions you made nitsanavni. Is there any improvement you see still? Please to Provide a PR if so ! 👍

martinsson · 2023-03-12T23:34:45Z

@emilybache FYI. I rewrote the test branch to alway keep the master branch mergeable into the with-tests branch. It's a convention that is useful when we make some modification to the master branche.
It still looks like your commit as I authored it in your name. Ping me if you're not ok with that.

emilybache · 2023-03-13T07:06:49Z

Hi,

Thanks Johan for fixing the git history so master can be merged into with_tests. I guess I was doing too much cherry-picking or something.

As for the sqlite thing - I have this branch in my repo with a version of the production code that also works with sqlite: https://github.com/emilybache/Refactoring-Kata-Lift-Pass-Pricing/blob/with_tests_sqlite3/python/src/prices.py it's even more horrible than the original and of course doesn't match the other language versions. It is platform independent though.

emilybache added 19 commits March 2, 2023 14:29

Basic flask application runs and has a unit test

81c8954

add gitignore

9f58166

additional tests

295b080

it connects to the db now too

9345250

tests in place, translation begun

f532dd2

tests passing

df34fe1

put is also working

53a6e9c

use same password as everyone else

05c138e

first attempt at github workflow

b51de77

attempt 2

50148b2

attempt 3

ce6cc8e

attempt 4

1760dc8

attempt 5

7a93a1c

attempt 6

b178bec

attempt 7

9754809

attempt 8

5203c63

attempt 9

31ec8a5

attempt 10

2bae589

attempt 11

48d46cd

nitsanavni reviewed Mar 4, 2023

View reviewed changes

.github/workflows/python-build.yml

on:

push:

paths:

- 'python/**'

Copy link

nitsanavni Mar 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😎

nitsanavni reviewed Mar 4, 2023

View reviewed changes

codecop mentioned this pull request Mar 4, 2023

bun.js typescript translation #62

Closed

emilybache added 6 commits March 9, 2023 09:00

first draft of documentation

bc74d2b

make db connection flexible enough to also use pymysql

15ed66a

make db connection use sqlite3 as a backup option

a48cd5a

Open and Close database connection properly on each request

fff09f9

improve the docs

2bc36a5

First attempt to get it to work on a system where mysql isn't installed

5e0f743

emilybache added 2 commits March 9, 2023 13:03

improve docs

cfd9bbb

wait for the server to start rather than sleeping a fixed time

0fbfe8c

martinsson mentioned this pull request Mar 12, 2023

Slight inconsistency in the python version #63

Open

martinsson merged commit 57cfc5d into martinsson:with_tests Mar 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python translation #61

Python translation #61

emilybache commented Mar 3, 2023

nitsanavni Mar 4, 2023

nitsanavni Mar 4, 2023

emilybache Mar 6, 2023

codecop Mar 6, 2023

nitsanavni Mar 7, 2023

nitsanavni Mar 4, 2023

emilybache Mar 6, 2023

nitsanavni Mar 4, 2023

nitsanavni Mar 4, 2023

nitsanavni Mar 4, 2023

emilybache Mar 6, 2023

codecop commented Mar 4, 2023

codecop commented Mar 4, 2023

nitsanavni commented Mar 4, 2023

martinsson commented Mar 5, 2023 •

edited

Loading

emilybache commented Mar 6, 2023

codecop commented Mar 6, 2023

emilybache commented Mar 9, 2023

martinsson commented Mar 12, 2023

martinsson commented Mar 12, 2023

emilybache commented Mar 13, 2023

Python translation #61

Python translation #61

Conversation

emilybache commented Mar 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecop commented Mar 4, 2023

codecop commented Mar 4, 2023

nitsanavni commented Mar 4, 2023

martinsson commented Mar 5, 2023 • edited Loading

emilybache commented Mar 6, 2023

codecop commented Mar 6, 2023

emilybache commented Mar 9, 2023

martinsson commented Mar 12, 2023

martinsson commented Mar 12, 2023

emilybache commented Mar 13, 2023

martinsson commented Mar 5, 2023 •

edited

Loading