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

feat: setup maintainable project #2

Merged
merged 18 commits into from
Dec 4, 2023
Merged

feat: setup maintainable project #2

merged 18 commits into from
Dec 4, 2023

Conversation

WilliamBergamin
Copy link
Contributor

Summary

This PR aims to introduce the necessary files to maintain this project

These change will enable maintainers to format, test, validate and deploy the project

Note: a number of these changes were brought from the Bolt and SDK projects

feedback

I'm looking for feedback on

  • Project structure
  • Potential typos
  • Any improvements (example: should we use something else then bash scripts to execute tasks)

@WilliamBergamin WilliamBergamin added the enhancement New feature or request label Dec 1, 2023
@WilliamBergamin WilliamBergamin self-assigned this Dec 1, 2023
@WilliamBergamin WilliamBergamin marked this pull request as ready for review December 1, 2023 18:57
@WilliamBergamin WilliamBergamin changed the title Setup project skeleton Setup maintainable project Dec 1, 2023
@WilliamBergamin WilliamBergamin changed the title Setup maintainable project feat: setup maintainable project Dec 1, 2023
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Pretty good! I was able to build/test/run using the instructions.

Would be nice to add the very basics from the maintainers guide to the README. Like, make sure you have python/pyenv installed, and how to run the tests, is probably sufficient to surface in the main README.

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Left a few minor comments. Great work!

jobs:
build:
runs-on: ubuntu-latest
timeout-minutes: 20
Copy link
Member

Choose a reason for hiding this comment

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

Does this require such a long time? We can make this as short as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point I've updated all the timeouts

jobs:
build:
runs-on: ubuntu-latest
timeout-minutes: 20
Copy link
Member

Choose a reason for hiding this comment

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

I know pytype can take long, but if a shorter duration is enough for this, we can adjust this value to make hunging ones to fail ealier.

pyproject.toml Outdated
Comment on lines 55 to 56
"ignore:\"@coroutine\" decorator is deprecated since Python 3.8, use \"async def\" instead:DeprecationWarning",
"ignore:The loop argument is deprecated since Python 3.8, and scheduled for removal in Python 3.10.:DeprecationWarning",
Copy link
Member

Choose a reason for hiding this comment

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

If you don't have any asyncio based code, these warnings won't arise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point 💯 removed these filter warnings

}

build() {
pip install -r requirements/build.txt && \
Copy link
Member

Choose a reason for hiding this comment

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

How about making the indent consistent within this script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch 💯

@WilliamBergamin WilliamBergamin merged commit e81b0c5 into main Dec 4, 2023
11 checks passed
@WilliamBergamin WilliamBergamin deleted the package-setup branch December 4, 2023 15:59
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.

3 participants