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

Isolate classes from Base to separate files #716

Closed
wants to merge 5 commits into from
Closed

Isolate classes from Base to separate files #716

wants to merge 5 commits into from

Conversation

alisnic
Copy link

@alisnic alisnic commented May 22, 2013

I was looking at Sinatra source code, and tried to understand how it works. The first thing I noticed was "there's SO MUCH STUFF in base.rb", seriously it's pretty easy to get lost there.

I isolated some of the classes from base to separate files up to the point when it would require to fiddle with the project object hierarchy to proceed. base.rb has shrunk with ~800 LOC.

This is my first attempt at a open source contribution like this, so I will understand if you will consider my efforts pointless.

Edit: The Travis build is failing, but it's not related to my changes as I see it. The errors that Travis throws are related to failure to get a gem, but I didn't touch the Gemfile. rake spec gives no failures for my changes.
https://travis-ci.org/sinatra/sinatra/jobs/7388775
https://travis-ci.org/sinatra/sinatra/jobs/7388777

@patriciomacadden
Copy link
Member

As @rkh said, there are no plans for this until 2.0.

See #658.

@alisnic
Copy link
Author

alisnic commented May 22, 2013

@patriciomacadden, Is there anything else I can do to help the project on this one? Or the idea is to just wait for 2.0 and then proceed?

@patriciomacadden
Copy link
Member

I think that there are no plans for 2.0 yet.

@patriciomacadden
Copy link
Member

You could help on sinatra-contrib or sinatra-recipes.

@alisnic
Copy link
Author

alisnic commented May 22, 2013

@patriciomacadden thanks, I will look at the projects.

There's no point in keeping this PR open, right?

@patriciomacadden
Copy link
Member

No, but thanks anyway for your time!

@alisnic alisnic closed this May 22, 2013
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.

None yet

2 participants