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

Addition of end command and greetings.py #3

Merged
merged 13 commits into from
Aug 28, 2021
Merged

Addition of end command and greetings.py #3

merged 13 commits into from
Aug 28, 2021

Conversation

chraem
Copy link
Member

@chraem chraem commented Aug 27, 2021

End Command in utils.py

  • Allows the termination of the bot in a shorter time. This will help developers that run their programs without IDE.

Test.py

  • This is a test file.
  • Provides a basic how-to of text embeddings and mentioning a user (currently the author of the message).

@huenique huenique self-requested a review August 27, 2021 14:15
@huenique
Copy link
Member

Almost there, just need to go over the syntax/code guideline. It's kinda important that we identify and resolve any syntax and style issues. Could you run black and isort against the dayong directory, like so

black dayong
isort --profile black dayong

Then try running flake8 and pylint

flake8 dayong
pylint dayong

@chraem
Copy link
Member Author

chraem commented Aug 27, 2021

Will do that, thanks.

@huenique
Copy link
Member

if there are issues, changes should be made and committed. something like "refactor: enforce code style" should be enough. thanks

@chraem
Copy link
Member Author

chraem commented Aug 28, 2021

  • Done running my code in black, isort, flake8, and pylint; it went smoothly but still have this syntax suggestion by pylint:

    dayong\bot.py:108:8: E0237: Assigning to attribute 'members' not defined in class slots (assigning-non-slot)

  • Fixed the issue with embedding emojis.

  • Added the actual greeting feature, greetings.py

  • Already test the .end command. The .end command is not visible to non-owners, even they typed .help.

Copy link
Member

@huenique huenique left a comment

Choose a reason for hiding this comment

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

The changes in the README file aren't quite right.

5. Run poetry shell. This will create a virtual environment and finds the pyproject.toml in the current directory.

6. Run poetry install. This will install all the dependencies listed in poetry.lock.

Number 5 doesn't make sense here. When poetry is run with the shell argument, it first finds the pyproject.toml file and creates the virtual environment based on that. However, if a virtual environment already exists for said file, it only needs to start it.

Number 6 is missing a few details but I don't suggest adding them. The pyproject.toml file designates the dayong directory as a package (name = "dayong"), hence everything inside it will be installed in the dist-packages or site-packages when poetry install is run. For the package dependencies, poetry installs everything in the poetry.lock file, but if that file doesn't exist, it uses pyproject.toml and then generates poetry.lock.

For now, I think it's best to revert the changes made to the README. Since it seems opinionated, I'd like to add my two cents. I prefer keeping it short and simple. Moreover, it's really not necessary for the reader to immediately know how poetry works as it isn't totally related. If they are interested, poetry's repository is already linked.

@huenique
Copy link
Member

huenique commented Aug 28, 2021

only remaining issue is with the readme file, everything else looks good. will merge once resolved 🙏

@chraem
Copy link
Member Author

chraem commented Aug 28, 2021

I've already edited the README, thanks for your correction 😊

Copy link
Member

@huenique huenique left a comment

Choose a reason for hiding this comment

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

really well done ✨

@huenique huenique merged commit 5cd9d6c into main Aug 28, 2021
@chraem chraem changed the title Addition of end command and test.py Addition of end command and greetings.py Aug 29, 2021
@huenique huenique added the enhancement New feature or request label Oct 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants