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.

Sign up for GitHub

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

Jump to bottom

Refactor Robyn and SubRouter classes #1072

Closed
wants to merge 6 commits into from
Closed

Refactor Robyn and SubRouter classes #1072

wants to merge 6 commits into from

Conversation

dave42w
Copy link
Contributor

@dave42w dave42w commented Dec 6, 2024 •
edited
Loading

Refactor both Robyn and SubRouter to be cleaner and leaner without changing the developer API at all.
Use unit tests to confirm the existing behaviour continues to work unchanged.

Description

This PR fixes #1071

Summary

This PR does the following:

  • added tests to create instances of Robyn and SubRouter with empty routing tables. This confirms that the doc examples still work.
  • added a test of the app.directory_path (I was having problems with test_cli, this is to catch any regression)
  • split the two big classes Robyn and SubRouter into a new module robyn_app.py

PR Checklist

Please ensure that:

  • The PR contains a descriptive title
  • The PR contains a descriptive summary of the changes
  • You build and test your changes before submitting a PR.
  • You have added relevant documentation
  • You have added relevant tests. We prefer integration tests wherever possible

Pre-Commit Instructions:

@dave42w
test: add basic unit tests to be sure we can create Robyn and SubRout…
7dd2bc9 
…er with empty lists of routes. This will confirm the imports haven't changed from the docs
@vercel Vercel
Copy link

vercel bot commented Dec 6, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
robyn ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 19, 2024 4:07pm

@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Dec 6, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #1072 will not alter performance

Comparing dave42w:Robyn-classes (ae27f31) with main (7495342)

Summary

✅ 146 untouched benchmarks

@dave42w dave42w marked this pull request as ready for review December 6, 2024 12:17
@dave42w
Copy link
Contributor Author

dave42w commented Dec 6, 2024

I'm not going to actually refactor the two classes until we get this basic move into a new module merged (it's going to create conflicts with other PRs if we don't get these merged in small bits.

sansyrox
sansyrox reviewed Dec 9, 2024
View reviewed changes
robyn/robyn_app.py
Copy link
Member

Choose a reason for hiding this comment

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

Hey @dave42w 👋

Thank you for the PR but I am not a big fan of moving these classes outside the main __init__.py file. I'd want the Robyn class to be present in the main file only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,
I'd be interested to understand better the reasons.
Is that just Robyn?
If I create a RobynBase class for Robyn and SubRouter where would that be best to go?
Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @sansyrox
Can I have some direction on this. There is some neat refactoring possible to simplify Robyn and SubRouter and reduce potential issues. But I'm not sure how you want this to be organised.

It seems messy to me to have all this in init_.py but also odd to move RobynBase elsewhere while leaving Robyn behind.

But I'm willing to go in steps that you are happy with.

@dave42w dave42w closed this Dec 31, 2024
@dave42w dave42w deleted the Robyn-classes branch December 31, 2024 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sansyrox sansyrox sansyrox left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Idea: refactor Robyn & SubRouter
2 participants
@dave42w @sansyrox