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

Allow underscores in routes #1713

Merged
merged 12 commits into from
Sep 8, 2023
Merged

Allow underscores in routes #1713

merged 12 commits into from
Sep 8, 2023

Conversation

ElijahAhianyo
Copy link
Collaborator

Description

This pull request addresses the need for converting underscores to hyphens in routes, which was previously implemented. The goal of this change is to enhance the flexibility of route naming by allowing both underscores and hyphens to coexist without the need for any conversions. This PR proposes to remove the underscore-to-hyphen conversion logic, allowing developers to freely choose between these two naming conventions for routes.

Changes Made

  • Removed the underscore-to-hyphen conversion logic from route handling, enabling route names to support both underscores and hyphens directly.

Motivation

  • Allowing both underscore-separated and hyphen-separated route names provides developers with greater flexibility in choosing naming conventions that align with their preferences and practices.
  • By removing the conversion, we empower developers to use the naming convention that best suits their needs, contributing to a more adaptable and user-friendly codebase.

Impact

With this change, developers are free to use both underscores and hyphens as they see fit in route names.
The removal of the underscore-to-hyphen conversion ensures that developers can choose their preferred naming convention without the need for special handling.
Existing routes with either underscores or hyphens in their names will continue to function as expected.

@ElijahAhianyo ElijahAhianyo changed the title Allow underscores in routes: Allow underscores in routes Aug 30, 2023
masenf
masenf previously approved these changes Aug 30, 2023
Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

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

Any plans to also make upper case letters work? That seems to come up about every 2-3 weeks on discord.

@ElijahAhianyo
Copy link
Collaborator Author

Any plans to also make upper case letters work? That seems to come up about every 2-3 weeks on discord.

Yeah, could add that to this PR

reflex/utils/format.py Show resolved Hide resolved
integration/test_event_chain.py Outdated Show resolved Hide resolved
Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

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

Looks good! I think we just need to update the integration tests in integration/test_dynamic_routes.py

@ElijahAhianyo ElijahAhianyo force-pushed the elijah/underscores_in_routes branch from 8b015b6 to 8f5b1e7 Compare September 7, 2023 21:54
app_harness_env: Type[AppHarness], tmp_path_factory
app_harness_env: Type[AppHarness], tmp_path_factory
Copy link
Collaborator

Choose a reason for hiding this comment

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

i don't understand why these blocks are all indented more...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

probably as a result of running precommits (black), not typically sure why those parts are affected

Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

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

looks good!

@picklelo picklelo merged commit 891e6a4 into main Sep 8, 2023
34 checks passed
@picklelo picklelo deleted the elijah/underscores_in_routes branch September 16, 2023 00:27
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.

3 participants