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.

Sign up for GitHub

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

Jump to bottom

Removed setTo argument from xPoweredBy #226

Merged
merged 6 commits into from
Jul 10, 2020
Merged

Removed setTo argument from xPoweredBy #226

merged 6 commits into from
Jul 10, 2020

Conversation

ameen-abdeen
Copy link

Removed XPoweredByOptions and make xPoweredBy take no arguments.
Removed the test
Updated the changelog and linked it to the given wiki page

EvanHahn
EvanHahn requested changes Jul 7, 2020
View reviewed changes
Copy link
Member

@EvanHahn EvanHahn 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 doing this! I left a few comments. Let me know if I can help further.

middlewares/x-powered-by/index.ts Outdated Show resolved Hide resolved
middlewares/x-powered-by/CHANGELOG.md Outdated Show resolved Hide resolved
middlewares/x-powered-by/CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
middlewares/x-powered-by/CHANGELOG.md Outdated Show resolved Hide resolved
@ameen-abdeen
Copy link
Author

Thanks for doing this! I left a few comments. Let me know if I can help further.

It is my pleasure to contribute to the world of opensource . There was a merge conflict in middlewares/x-powered-by/CHANGELOG.md which I have resolved but not sure whether it is right. There was 2.0.0 - Unreleased tag. I was not quite sure whether my change was required under it

EvanHahn
EvanHahn requested changes Jul 8, 2020
View reviewed changes
middlewares/expect-ct/index.ts Outdated
@@ -21,12 +21,12 @@ function parseMaxAge(value: void | number): number {
function getHeaderValueFromOptions(options: Readonly<ExpectCtOptions>): string {
const directives: string[] = [];

directives.push(`max-age=${parseMaxAge(options.maxAge)}`);
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be done as part of this pull request. Could you revert? We should also revert the changes in test/expect-ct.test.ts.

(It can also be done against the master branch instead of the 4.x branch.)

package.json Outdated Show resolved Hide resolved
middlewares/x-powered-by/CHANGELOG.md Show resolved Hide resolved
@EvanHahn
Copy link
Member

EvanHahn commented Jul 9, 2020

Thanks for your help here! I will take a look tomorrow.

EvanHahn
EvanHahn approved these changes Jul 9, 2020
View reviewed changes
Copy link
Member

@EvanHahn EvanHahn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good! Thank you!!

Could you fix the merge conflicts? Once you do that, I'll merge. Let me know if you need help with that.

I'm going to merge this now. Feel free to add yourself to the list of contributors in package.json in a separate pull request.

@ameen-abdeen
Copy link
Author

Looks good! Thank you!!

Could you fix the merge conflicts? Once you do that, I'll merge. Let me know if you need help with that.

I'm going to merge this now. Feel free to add yourself to the list of contributors in package.json in a separate pull request.

I don't see any merge conflicts between helmetjs:4.x and ameen-abdeen:master.

@EvanHahn EvanHahn merged commit 38e695d into helmetjs:4.x Jul 10, 2020
@EvanHahn
Copy link
Member

My mistake. Just merged this! Thanks so much!

Would you like me to add you to the contributors in package.json? If so, what name, email, and website should I use? (Email and website are optional.)

@EvanHahn EvanHahn mentioned this pull request Jul 10, 2020
Closed
@ameen-abdeen
Copy link
Author

My mistake. Just merged this! Thanks so much!

Would you like me to add you to the contributors in package.json? If so, what name, email, and website should I use? (Email and website are optional.)

That would be great. Name: Ameen Abdeen Email: [email protected]. I don't have a website yet.

EvanHahn added a commit that referenced this pull request Jul 10, 2020
@EvanHahn
Add Ameen Abdeen to the contributors list
e1746c1 
See [this comment][0].

[0]: #226 (comment)
@EvanHahn
Copy link
Member

Credited you in e1746c1! Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EvanHahn EvanHahn EvanHahn approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ameen-abdeen @EvanHahn