Skip to content

Reply/feature#8

Open
s0h3ck wants to merge 4 commits intoranihorev:masterfrom
s0h3ck:reply/feature
Open

Reply/feature#8
s0h3ck wants to merge 4 commits intoranihorev:masterfrom
s0h3ck:reply/feature

Conversation

@s0h3ck
Copy link
Copy Markdown

@s0h3ck s0h3ck commented Jun 24, 2020

• Feature to edit a reply (reply blueprint)
• Update README.md
• Remove duplicated configuration in .env_example

Copy link
Copy Markdown
Owner

@ranihorev ranihorev left a comment

Choose a reason for hiding this comment

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

First of all, thanks for updating the docs!
The code looks good, my comments are mainly around how to organize the codebase.

We'll also need to update the frontend to support that btw

Comment on lines +13 to +20
### Install pgadmin (optional)

Change the default settings.

```
podman run --name pgadmin -p 5050:80 -e "PGADMIN_DEFAULT_EMAIL=pgadmin4@pgadmin.org" -e "PGADMIN_DEFAULT_PASSWORD=admin" -d dpage/pgadmin4
```

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I don't think that this is necessary here

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 is optional. At least, people will know it is an option they can use and explore. It is a simple podman command to get started. Ideally, you would have a Dockerfile with a docker-compose file in this project. The setup and configuration time would have been saved for anyone who wants to contribute to this project.

For your information, Podman 2.0 was released less than one month ago.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I agree in regards to Docker :)
#9


EMPTY_FIELD_MSG = 'This field cannot be blank'

class ReplyResource(Resource):
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It might be confusing to have 2 ReplyResource in the code base. I suggest consolidating the two resources and replacing the post method below with patch

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It'll also ensure that we won't have two copies of replies_fields

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.

I agree with you. I would suggest the owner of the code to clean and remove unused code because it can be confusing. For example, MongoDB seems to be replaced with PostgreSQL. Also, new_backend, main, app, src in the same project makes it a little bit confusing. I think one common convention is app/main.py.

Copy link
Copy Markdown
Owner

@ranihorev ranihorev Jul 19, 2020

Choose a reason for hiding this comment

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

You're absolutely right (and I apologize about that), the code is still patchy but we're working on fixing that.
Would you like to update your code and we'll take care of re-organizing the rest?

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.

2 participants