-
-
Notifications
You must be signed in to change notification settings - Fork 308
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
Make the movie lister example work on Windows #458
base: develop
Are you sure you want to change the base?
Conversation
…l newline, which made the example fail on Windows. - Changed the relative imports to absolute imports so the example works out of the box in VS Code. - Added a shell script and a batch file to run the example on Linux and Windows.
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.
Hey @jpvandervelden :) Thanks a lot for the contribution and glad you've figured it out how to run it on windows. I posted a couple of actions. Appreciate if you could take a look.
@@ -4,8 +4,8 @@ | |||
|
|||
from dependency_injector.wiring import inject, Provide | |||
|
|||
from .listers import MovieLister | |||
from .containers import Container | |||
from movies.listers import MovieLister |
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.
I looked for any specific behavior of relative imports on Windows, but didn't find anything. Seems like you've had a problem with it. Do you remember how you ran it and what was the problem?
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 was a problem when using VS Code..
@@ -0,0 +1,11 @@ | |||
echo on | |||
set PYTHON37=%UserProfile%\AppData\Local\Programs\Python\Python37\python.exe |
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.
I'm unsure if it's ok to use a specific version of Python in the documentation. The idea is that this example should work for all supported Python versions.
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 line could be removed and a comment added that the assumption is made that PATH environment variable is pointing to the python installation.
|
||
%PYTHON37% -m venv %ENV_NAME% | ||
|
||
Start /WAIT cmd /k "%VENV_DIR%\activate & %PYTHON_VENV% -m pip install --upgrade pip & %PYTHON_VENV% -m pip install -r .\requirements.txt & set MOVIE_FINDER_TYPE=csv& %PYTHON_VENV% -m movies & set MOVIE_FINDER_TYPE=sqlite& %PYTHON_VENV% -m movies&%VENV_DIR%\deactivate & pause & exit 0" |
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.
Is it possible to split this line to multiple? Kind of hard to scroll it now
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.
Yes.. it is, by adding a ^as a multiline command separator.
I will check in the file again.
|
||
rm -rf wslenv2 | ||
|
||
python3 -m venv wslenv2 |
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.
So, this is basically doing what is done in the readme, correct?
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.
Correct.
@@ -18,10 +18,9 @@ | |||
|
|||
|
|||
def create_csv(movies_data, path): | |||
with open(path, 'w') as opened_file: | |||
with open(path, 'w', newline='') as opened_file: |
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.
Do you remember what error you got before making this change?
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.
The error message was:
TypeError: __init__() missing 3 required positional arguments: 'title', 'year', and 'director'
which was caused by the CSV data being created with extra carriage return and new line.
The movies.csv file was like this:
The Hunger Games: Mockingjay - Part 2,2015,Francis Lawrence
Rogue One: A Star Wars Story,2016,Gareth Edwards
The Jungle Book,2016,Jon Favreau
@@ -72,3 +72,12 @@ src/dependency_injector/providers/*.so | |||
|
|||
# Workspace for samples | |||
.workspace/ | |||
/examples/miniapps/movie-lister/wslenv2-win/Scripts |
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.
I'm unsure we need these imports here
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 is how the virtual environment is created under Windows.
Changed script to use multiline command separator for the long command sequence. Added some comment lines.
As a Java guy I'd like to use the dependency injection in Python too. I found this library. It didn't work for me at first on Windows & VS Code. I made some changes so now it works on Linux and Windows.
Keep up the good work!