Skip to content

API domain can be configured - #5

Closed
immer wants to merge 1 commit into
tegon:masterfrom
immer:master
Closed

API domain can be configured#5
immer wants to merge 1 commit into
tegon:masterfrom
immer:master

Conversation

@immer

@immer immer commented Mar 20, 2016

Copy link
Copy Markdown

So that this library can be used with GitHub Enterprise,
Organization.prototype.BASE_URI will either read an environment variable
(ENTERPRISE_DOMAIN), or fallback to GitHub's public domain.

So that this library can be used with GitHub Enterprise,
Organization.prototype.BASE_URI will either read an environment variable
(ENTERPRISE_DOMAIN), or fallback to GitHub's public domain.
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.1%) to 81.061% when pulling 63b368d on rwassey:master into e886061 on tegon:master.

@immer

immer commented Mar 20, 2016

Copy link
Copy Markdown
Author

Just as a note, I don't consider this branch ready for master. I hope you can see where I'm going.

Any feedback would be welcome when you have time.

Cheers

Comment thread lib/github.js
var github = exports;

function Organization(options) {
this.BASE_URI = 'https://api.github.com/orgs/';

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never considered GitHub Enterprise, but it makes sense to support it somehow. Your solution is good, but I think we don't need a function for that. You could just change this line to:

this.BASE_URI = process.env.ENTERPRISE_DOMAIN || 'https://api.github.com/orgs/'

What to you think?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your solution is probably more simple. Although I have an aversion to any logic living in constructors. It's just a force of habit to make things more testable.

Having said that, I'll make the change and resubmit.

Cheers!

@dalelotts

Copy link
Copy Markdown

@tegon I think this can be closed. If you like, I can add this functionality to PR #14 it will be very easy.

@immer immer closed this Mar 6, 2020
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.

4 participants