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

explicitly require "onelogin/ruby-saml/logging" #134

Merged
merged 1 commit into from Oct 10, 2014

Conversation

nkeyes
Copy link
Contributor

@nkeyes nkeyes commented Jun 2, 2014

so that the developer may do:

require 'onelogin/ruby-saml/authrequest'

instead of

require 'onelogin/ruby-saml'

or

require 'onelogin/ruby-saml/authrequest'
require 'onelogin/ruby-saml/logging'

… may do:

require 'onelogin/ruby-saml/authrequest'

instead of
require 'onelogin/ruby-saml'

or
require 'onelogin/ruby-saml/authrequest'
require 'onelogin/ruby-saml/logging'
@Lordnibbler
Copy link
Contributor

Can you add documentation for this feature, @nkeyes ?

@nkeyes
Copy link
Contributor Author

nkeyes commented Sep 9, 2014

I'm not sure what documentation is needed or where it would go, so I'll explain what I did here.
All I did was add the proper require statements so that one can directly require 'onelogin/ruby-saml/authrequest' without an error.

The project I was working on only needed OneLogin::RubySaml::Authrequest, but to get that I'd either have to require 'onelogin/ruby-saml', unnecessarily loading loading the whole library, or require authrequest, and logging. OneLogin::RubySaml::Authrequest depends on OneLogin::RubySaml::Logging, so it should be required from that file.

@Lordnibbler
Copy link
Contributor

@nkeyes Can you just add the information from your pull request description into the readme.md?

@nkeyes
Copy link
Contributor Author

nkeyes commented Sep 9, 2014

Yeah, I can do that later tonight.

@Lordnibbler
Copy link
Contributor

Great we will merge after that!

@Lordnibbler
Copy link
Contributor

@nkeyes just following up --

@luisvm @pitbulk @inakidelamadrid can you guys review? this is 👍 from me after documentation is added

@pitbulk
Copy link
Collaborator

pitbulk commented Oct 9, 2014

After merge this pull-request I think a message could be added to the Readme.md (since there is no reference to any requires in the README.md),

I think one place to set it is at the Overview section, before the "The initiation phase". Something like:

In order to use the toolkit you will need to add the library in your ruby project.

you can add the whole toolkit:
require 'onelogin/ruby-saml'
or just the required part:
require 'onelogin/ruby-saml/authrequest'

What do you think?

@Lordnibbler , can you merge it and commit this proposed doc or any?

@Lordnibbler
Copy link
Contributor

@pitbulk sure will do that now.

Lordnibbler added a commit that referenced this pull request Oct 10, 2014
explicitly require "onelogin/ruby-saml/logging"
@Lordnibbler Lordnibbler merged commit 9920d9b into SAML-Toolkits:master Oct 10, 2014
@Lordnibbler
Copy link
Contributor

@pitbulk done via b7ee3d7

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

3 participants