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

Require reflexes to be defined in a reflex.py module or a reflexes package for simpler discovery. #62

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mekhami
Copy link
Contributor

@mekhami mekhami commented Dec 25, 2020

Type of PR (feature, enhancement, bug fix, etc.)

Enhancement

Description

Please include a summary of the change and which issue is fixed.

  1. load_reflexes_from_config now looks for a reflexes.py module or a reflexes package which exposes its package members directly, instead of walking the filepath. This will be more stable in the long run, and adheres to idioms from the Django framework.
  2. Updated the example project to reflect the new requirement
  3. Updated the docs to be more clear about where/how to define reflexes.
  • Tests are passing
  • Documentation has been added or amended for this feature / update

    1. Update requirements_dev.txt to include base requirements, as
       these are necessary for starting the server and performing any tests.

    2. Use Reflex.__subclasses__() rather than name checking for reflex
       subclass detection, which will be more consistent and allow users
       to name their reflexes whatever they want.

    3. Clear falsey values from the filepaths detected in the
       load_reflexes_from_config function, which is a tenuous solution to
       not including vim-generated dotfiles in the module loading process.

undo style changes
…package for ease of discovery.

flake8 errors
@DamnedScholar
Copy link
Contributor

The current scaffolding system generates a blank __init__.py file, so the scaffold would be broken out of the box with this change.

I appreciate the idiomatic change, but having to remember to fill out the __init__.py every time I add a new bit of functionality might make me want to say shove it to code organization and shove all of my Reflexes in one uber-file. Since the scope of the reflexes package would be limited and predictable, and since we can assume that users want access to every reflex they define, I feel like this would be the opportunity to do something fancy where __init__.py dynamically imports every file in the folder. Most users wouldn't ever have to think about the code in that file, just making sure they have Reflex-descended classes inside it.

@mekhami
Copy link
Contributor Author

mekhami commented Dec 25, 2020

I can fix the scaffolding change, thanks for bringing that to my attention.

Personally, given that this paradigm is already established in Django, and it's as close to basic python as it gets, I'd rather not do anything terribly magical in __init__.py.

I think my preference would be to have the default scaffolding generate a reflexes.py file instead of a reflexes package, and document the option to the user of turning it into a package a la the django model documentation. "Simple is better than complex." and all that.

@jonathan-s
Copy link
Owner

jonathan-s commented Dec 25, 2020

I think if we're going for a breaking change here it could be interesting to use a registering logic. There's less chance that you'd forget about that since you'd need to do it in the same file, just above the reflex class.

Think the same as what is done in django admin -> @admin.register(ModelClass). I wrote briefly about that in #26. To me that would be pretty idiomatic django as well.

@JulianFeinauer
Copy link
Contributor

I like the discussion here and Indeed I think I would prefer something simple and explicit as the registration @jonathan-s suggests. I dont know enough python / django to comment on the discussion of @mekhami and @DamnedScholar who seem to be more in the matter.

@JulianFeinauer
Copy link
Contributor

And just as a side note.. i came across the problem, that I wanted to check againt a "registry" if a reflex class / method is available (the idea is to check if a given template tag references a valid reflex). So I think it would definetly be good to make a registry somewhat standalone (currently its pretty implicit in the consumer).

@DamnedScholar
Copy link
Contributor

DamnedScholar commented Dec 26, 2020

I like a registry. It also fits a Django idiom and gives you a chance to have your view functions check for functionality that might make or break the view. It would also allow for multiple organization strategies, including the magic folder and having a Reflex class living in the same file as other classes that define the same functionality.

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.

4 participants