Skip to content
This repository was archived by the owner on Jan 25, 2020. It is now read-only.

Dev#48

Open
mvartanyan wants to merge 250 commits intomasterfrom
dev
Open

Dev#48
mvartanyan wants to merge 250 commits intomasterfrom
dev

Conversation

@mvartanyan
Copy link
Copy Markdown

No description provided.

Kvm99 and others added 27 commits August 18, 2019 19:53
…nto feature/rss

# Conflicts:
#	app/__init__.py
#	app/templates/base.html
#	config.py
…nto feature/rss

# Conflicts:
#	app/rss.py
#	app/templates/base.html
#	config.py
…nto feature/rss

# Conflicts:
#	app/templates/base.html
Comment thread Dockerfile-flask
COPY . .
# Finally, we run uWSGI with the ini file we
# created earlier
CMD [ "uwsgi", "--ini", "app.ini" ] No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No new line at end of file

Comment thread Dockerfile-nginx
RUN rm /etc/nginx/conf.d/default.conf
# We copy the requirements file in order to install
# Python dependencies
COPY app.conf /etc/nginx/conf.d No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No new line at end of file

Comment thread app.conf
include uwsgi_params;
uwsgi_pass flask:5000;
}
} No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No new line at end of file

Comment thread app.ini
; then expose on our Dockerfile
socket = 0.0.0.0:5000
vacuum = true
die-on-term = true No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No new line at end of file

Comment thread app/forms.py
class EpisodeUploadForm(FlaskForm):
file = FileField('Upload podcast', validators=[
FileRequired(),
FileAllowed(['mp3'], "Wrong format! Only mp3 format audio files")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do you use different quotes style?

Comment thread app/tests.py
@@ -0,0 +1 @@
# Here will be tests
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Where are tests? :)

Comment thread config.py
@@ -0,0 +1,49 @@
# Config Classes
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Useless comment

Comment thread migrations/README
@@ -0,0 +1 @@
Generic single-database configuration. No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No new line at end of file

Comment thread tests/test_audio_utils.py
test_audio_path = os.path.join(
script_path,
"tests",
"test_data",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use one quote style (single quotes)

Comment thread tests/test_audio_utils.py
test_audio_path = os.path.join(
script_path,
"tests",
"test_data",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use one quote style (single quotes)

Comment thread .gitignore
# Spyder project settings
.spyderproject
.spyproject
yandex_tokens.py
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

are we using yandex tokens in the final version? 

Comment thread .gitignore

# audio files:
app/media/
media/
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It's not a great idea to store files created by your code in the same directories as the code itself

Comment thread Dockerfile-flask
@@ -0,0 +1,24 @@
# We simply inherit the Python 3 image. This image does
# not particularly care what OS runs underneath
FROM python:3
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

why not use a ready-made flask image? 

Comment thread HOW TO RUN FLASK APP.md
flask db migrate
flask db upgrade

flask run
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

flask run is only good enough for development. More on this https://flask.palletsprojects.com/en/1.1.x/tutorial/deploy/#run-with-a-production-server

Comment thread README.md
12. Frontend: Bootsrap + jinja
## Eriwan
-------------
Web servis for public podcast creation where each user can add episodes and
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Spelling. I will only make this comment once about human-readable content here, though spelling is not great across the board. Spelling is very important. Misspelled words will not be found by search, making navigation in your code, comments and documentation very difficult.

Comment thread app/tests/test_rss.py
assert p.explicit
assert p.description == 'Eriwan_Podcast'
assert p.website == 'localhost'
assert p.file.name == 'feed_template.xml'
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

why would you test a constant? 

Comment thread config.py
MAX_CONTENT_LENGTH = 20 * 1024 * 1024
UPLOAD_PODCAST_FOLDER = 'episodes'

DROPZONE_DEFAULT_MESSAGE = 'Перетащите аудио файл в эту зону для загрузки'
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

So there is a concept of detached string literals in the application, it is just not used consistently across the application.

Comment thread config.py

HOST = os.environ.get('HOST', 'localhost:5000')

MAX_CONTENT_LENGTH = 20 * 1024 * 1024
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Magic happens here!

Comment thread migrations/env.py
@@ -0,0 +1,96 @@
from __future__ import with_statement
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Here and in some other places: is it actually necessary to have foreign code in our repo? Is there a better way of handling this? 

Comment thread tests/test_audio_utils.py


def test_concatenate_audios():
script_path = path.abspath(path.join(os.getcwd()))
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

not guaranteed to be writeable

Comment thread app/errors.py
@@ -0,0 +1,15 @@
# Handle route and server exceptions
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

useless comment

Comment thread app/models.py
Return wrapped in jingles file path
'''
media_path = os.path.join(app.config.get('MEDIA_ROOT'), 'episodes')
file_path = f'{media_path}/{self.id}.mp3'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To everybody: Pay attention to this problem! Use os.path.join ONLY for string concatenating!!!!!!!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.