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

Make the movie lister example work on Windows #458

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

jpvandervelden
Copy link

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!

…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.
Copy link
Member

@rmk135 rmk135 left a 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
Copy link
Member

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?

Copy link
Author

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
Copy link
Member

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.

Copy link
Author

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"
Copy link
Member

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

Copy link
Author

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
Copy link
Member

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?

Copy link
Author

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:
Copy link
Member

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?

Copy link
Author

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
Copy link
Member

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

Copy link
Author

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.

@rmk135 rmk135 changed the base branch from master to develop June 4, 2021 23:58
@rmk135 rmk135 self-assigned this Jun 4, 2021
Changed script to use multiline command separator for the long command sequence.
Added some comment lines.
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

2 participants