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

Huge cleaning and refactoring #33

Merged
merged 4 commits into from
Mar 8, 2012
Merged

Huge cleaning and refactoring #33

merged 4 commits into from
Mar 8, 2012

Conversation

rymai
Copy link
Collaborator

@rymai rymai commented Mar 6, 2012

Tests unchanged so there should be no regression.

Here is the summary of my changes:

  • Get rid of the "env" param and use @request everywhere
  • Extracted constraint matching in a new SslEnforcerConstraint class
  • Simplified logic by x10000! ;)

Next step is to add specific tests for SslEnforcerConstraint...

Let me know guys what you think (so I can merge it myself, or not)! :)

…regression.

- Get rid of the "env" param and use @request everywhere
- Extracted constraint matching in a new dedicated class
- Simplified logic by x10000! ;)

Next step is to add specific tests for SslEnforcerConstraint...
@rymai
Copy link
Collaborator Author

rymai commented Mar 7, 2012

Also, what do you think about replacing ssl-enforcer by ssl_enforcer in all files & directories (except lib/rack-ssl-enforcer.rb itself)?

@thibaudgg
Copy link
Collaborator

nice, +1!

@tobmatth
Copy link
Owner

tobmatth commented Mar 7, 2012

Rymai,

thanks for your effort in cleaning this up, i'd be glad to see this merged in. Also +1 for the renaming...

@rymai
Copy link
Collaborator Author

rymai commented Mar 7, 2012

I've refactored the tests and improved/completed the README (and converted it to Markdown).

@rymai
Copy link
Collaborator Author

rymai commented Mar 7, 2012

@tobmatth Could you activate the Travis CI hook?

tobmatth added a commit that referenced this pull request Mar 8, 2012
@tobmatth tobmatth merged commit af10697 into master Mar 8, 2012
@tobmatth
Copy link
Owner

tobmatth commented Mar 8, 2012

Could you activate the Travis CI hook?

[x] done

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