Skip to content

500 - Internal Server Error#51

Merged
ErikssonF merged 10 commits intomainfrom
issue-20
Feb 8, 2022
Merged

500 - Internal Server Error#51
ErikssonF merged 10 commits intomainfrom
issue-20

Conversation

@albindubech
Copy link
Copy Markdown
Contributor

I think this resolves #20

@LordRekishi
Copy link
Copy Markdown
Contributor

I think you got conflicts because of the changes made in merging Issue #40 earlier today. Maybe if you merge in the new main branch it could be solved?

@albindubech
Copy link
Copy Markdown
Contributor Author

I think you got conflicts because of the changes made in merging Issue #40 earlier today. Maybe if you merge in the new main branch it could be solved?

Whould I need to create a new Pull Request for this to be made?

@LordRekishi
Copy link
Copy Markdown
Contributor

I think you got conflicts because of the changes made in merging Issue #40 earlier today. Maybe if you merge in the new main branch it could be solved?

Whould I need to create a new Pull Request for this to be made?

Exactly. Update the project through intellij, and then merge main into your branch.

@albindubech albindubech closed this Feb 3, 2022
@Philippevial
Copy link
Copy Markdown
Contributor

.

I think the pull-request updates when you push from the same branch again, so you don't need to create a new one!
(After updating the branch of course)

@albindubech albindubech reopened this Feb 3, 2022
@albindubech
Copy link
Copy Markdown
Contributor Author

This pull request should now be updated with the latest changes form main

@kappsegla
Copy link
Copy Markdown
Contributor

The HttpResponse class holding response header seams to be a good idea. Maybe call the class HttpResponseStatusCodes or something similar? And keep the HttpResponse class available for a complete response from the server?

Hardcoding a contact adress to fungover.org in the code is a bad idea, because this server should be able to run on many different hostnames, not just ours. Suggestion to just return 500 Internal Server Error because protocol version depends on how our server is configured so something else in the code has to define that.

@albindubech
Copy link
Copy Markdown
Contributor Author

The HttpResponse class holding response header seams to be a good idea. Maybe call the class HttpResponseStatusCodes or something similar? And keep the HttpResponse class available for a complete response from the server?

Hardcoding a contact adress to fungover.org in the code is a bad idea, because this server should be able to run on many different hostnames, not just ours. Suggestion to just return 500 Internal Server Error because protocol version depends on how our server is configured so something else in the code has to define that.

Should I rename the class to "HttpResponseStatusCodes" and leave the "HTTpResponse" name available for later use then? Or did I misunderstand?

@kappsegla
Copy link
Copy Markdown
Contributor

The HttpResponse class holding response header seams to be a good idea. Maybe call the class HttpResponseStatusCodes or something similar? And keep the HttpResponse class available for a complete response from the server?
Hardcoding a contact adress to fungover.org in the code is a bad idea, because this server should be able to run on many different hostnames, not just ours. Suggestion to just return 500 Internal Server Error because protocol version depends on how our server is configured so something else in the code has to define that.

Should I rename the class to "HttpResponseStatusCodes" and leave the "HTTpResponse" name available for later use then? Or did I misunderstand?

Much better name like this.

@Vimbayinashe
Copy link
Copy Markdown
Contributor

This looks good! It would be good to add tests for these changes as soon as #62 has received a second review.

@Vimbayinashe Vimbayinashe self-requested a review February 7, 2022 13:37
@albindubech
Copy link
Copy Markdown
Contributor Author

This looks good! It would be good to add tests for these changes as soon as #62 has received a second review.

Good suggestion, will update with tests when #62 is completed.

@Vimbayinashe
Copy link
Copy Markdown
Contributor

This looks good! It would be good to add tests for these changes as soon as #62 has received a second review.

Good suggestion, will update with tests when #62 is completed.

Pull request #62 is now marked as draft as it has become dependent on another issue. I will approve this pull request now.

Copy link
Copy Markdown
Contributor

@ErikssonF ErikssonF left a comment

Choose a reason for hiding this comment

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

Looks good!

@ErikssonF ErikssonF merged commit a41ebe8 into main Feb 8, 2022
@ErikssonF ErikssonF deleted the issue-20 branch February 11, 2022 08:37
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.

500 - Internal Server Error

6 participants