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

Refactoring codebase #56

Merged
merged 3 commits into from
Jun 11, 2020
Merged

Refactoring codebase #56

merged 3 commits into from
Jun 11, 2020

Conversation

alaxalves
Copy link
Member

@alaxalves alaxalves changed the title Refactoring codebase WIP: Refactoring codebase Jun 7, 2020
@alaxalves alaxalves force-pushed the fix/modularizing branch 2 times, most recently from 60dd170 to 48cb333 Compare June 7, 2020 23:50
@alaxalves alaxalves changed the title WIP: Refactoring codebase Refactoring codebase Jun 7, 2020
@icarito
Copy link
Member

icarito commented Jun 8, 2020

I'm not an expert Ruby programmer so I'm not sure what the best practices are here.
A comment that I think the headers belong in every file, you've moved them to the controller from app.rb.
If ruby developers prefer this structure it will be up the the development team although I wrote this app initially.
I'm not sure I understand the logic for the split, perhaps a note in the header to explain would help to understand what is where. Thanks.

@alaxalves
Copy link
Member Author

Here I'm just

I'm not an expert Ruby programmer so I'm not sure what the best practices are here.
A comment that I think the headers belong in every file, you've moved them to the controller from app.rb.
If ruby developers prefer this structure it will be up the the development team although I wrote this app initially.
I'm not sure I understand the logic for the split, perhaps a note in the header to explain would help to understand what is where. Thanks.

Here I'm just on phase one of use Sinatra's modular code styling. According to the Sinatra docs: “When a classic style application is run, all Sinatra::Application public class methods are exported
to the top-level.” Also, using the classical style prevents you from running more than
one Sinatra application per Ruby process – all calls to these top-level methods are handled by Sinatra::Application, functioning as a singleton. We can avoid these potentially-confusing scoping
problems by reorganizing our application into what Sinatra calls the “modular” style - which is what I'm starting to do here.

Besides mitigating scoping issues, we could reduce code duplication(by using lambdas) e some other advantages.

I have not done every changes needed to switch our app from classical to modular style, because this would be a huge PR. In this PR I made a few changes in a way that the code keeps running properly without the need to change anything. A couple more PRs will be required to fully modularize.

@jywarren
Copy link
Member

Hi, if Sinatra makes the recommendation of this code organization then I'm in support, thank you! It largely matches the Ruby on Rails style of directory structure. I agree with @icarito that perhaps we should link to the Sinatra recommendation by way of explanation, perhaps also in the readme?

Thank you!

@jywarren
Copy link
Member

I guess I'll go ahead and merge this and if you could drop in the link in the description at top that'd be great! Thanks!

@jywarren jywarren merged commit 8709207 into publiclab:main Jun 11, 2020
@jywarren
Copy link
Member

Hi @alaxalves just wanted to mention that in this repo, once the main branch is updated, the production container auto-reboots with the new code. 🎉

@alaxalves alaxalves deleted the fix/modularizing branch June 15, 2020 01:18
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