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

Issue-18/Models reinitialized without editable_attributes #30

Closed
wants to merge 1 commit into from

Conversation

dalvarez2596
Copy link

Issue 18

Description

  • Found that the problem was associated to the moment the (editable_attributes) was defined and when the models being reloaded In dev mode, so as we reload the models in every change, and because of the dynamic nature of editable attributes, the same were removed during each reload, until we restart the server.
  • In order to avoid this behaviour I add the method to_prepare, in order to reload everytime the models got reloaded.

I used these resources :
Initialization-events
Boot load reloadable

How it looks working:

Screen.Recording.2024-11-09.at.8.42.35.PM.mov

…e_attributes along with models on every change.
@dalvarez2596 dalvarez2596 requested a review from a team as a code owner November 10, 2024 03:11
@simon-isler
Copy link
Member

thank you for looking into it 🙌🏼

I don't really like how we define the editable_attributes so far. IMO there is no need to define it on existing models. I'd prefer a module with a method that returns the editable attributes for a model. This will make it also easier to test, which is essential since this logic is where everything starts :)

I'm currently refactoring it and will create a PR soon :)

@dalvarez2596
Copy link
Author

Yeah, honestly, I wasn’t a huge fan of that approach either. I was about to refactor it, but since this was my first open source PR, I wasn't 100% sure about it, so I just did a quick fix to get things running and make sure I could test without breaking stuff. I was actually thinking of using something like the strategy pattern or a factory, but didn’t want to touch a lot of stuff and complicate things too much.

Anyway, if there’s anything I can help with, just let me know! I’m really enjoying contributing to open source and have some free time to jump in on stuff. :).

@simon-isler
Copy link
Member

simon-isler commented Nov 12, 2024

Yeah, honestly, I wasn’t a huge fan of that approach either. I was about to refactor it, but since this was my first open source PR, I wasn't 100% sure about it, so I just did a quick fix to get things running and make sure I could test without breaking stuff. I was actually thinking of using something like the strategy pattern or a factory, but didn’t want to touch a lot of stuff and complicate things too much.

you're welcome to challenge stuff :) if you're unsure about a change you can ask us by opening an issue e.g.

Anyway, if there’s anything I can help with, just let me know! I’m really enjoying contributing to open source and have some free time to jump in on stuff. :).

that sounds great! #19 would be nice, do you want to look into it? I'll update the description just now

@simon-isler
Copy link
Member

I made a couple of fixes and refactorings in #31, #32 and #33. I'll therefore close this PR.

@dalvarez2596
Copy link
Author

I’ll take a look at the issue and let you know if I have any doubts or concerns. Thanks for adding a description :)
I just tested the PRs you made, and all three of them seem to be working fine. I didn’t find any issues.

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.

2 participants