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

Enable use in repositories owned by other users/orgs #1

Closed
wants to merge 3 commits into from

Conversation

leonardodino
Copy link

No description provided.

@@ -16,7 +16,7 @@
"scripts": {
"start": "node .",
"mocha": "NODE_ENV=test istanbul cover _mocha -- -u exports -R spec test/**/*.spec.js",
"dependency-check": "JS_FILES=\"*.js test/*.js\" && dependency-check . $JS_FILES && dependency-check . $JS_FILES --unused --no-dev",
"dependency-check": "JS_FILES=\"*.js test/*.js\" && dependency-check . $JS_FILES && dependency-check . $JS_FILES --unused --no-dev -i encoding",
Copy link

Choose a reason for hiding this comment

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

This change is outdated now :-)

Copy link
Owner

@voxpelli voxpelli left a comment

Choose a reason for hiding this comment

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

Thanks for the setup. The README says that user should be:

"[...] the username of the owner of the repository to publish to"

This should be changed as well to reflect that it should be your own/the token owner's username, the reason being the getBaseHeaders() method, which uses the user in the user-agent header.

I'll have to think about your implementation though. I don't really like the concatenated user + repository name in the repo variable.

I think that I would like to split a repo containing a / and save the two parts as repo and repoOwner, with repoOwner defaulting to the value of user at all other times. Then I would use repoOwner instead of user in the originalgetPath().

What do you think? (And sorry for the late reply!)

@@ -39,7 +39,7 @@
"nock": "^9.0.0"
},
"dependencies": {
"node-fetch": "^1.3.0",
"node-fetch": "^2.1.2",
Copy link
Owner

Choose a reason for hiding this comment

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

This change is unrelated and should be done on its own as a separate PR

@leonardodino
Copy link
Author

Totally forgot I made this pull request based on the master of my fork. 😂🤦‍♂️

The relevant changes are all in the first commit

the rest is specific to some troubles i had on webpack + aws lambda,
and are already outdated, as noted bt @strugee.

I agree that when considering the instance own properties, this is a breaking/incorrect change, but so is introducing another parameter so i don't know what is the correct way.

This stands more as a feature request than a complete PR, i guess. (:
nice work!

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