-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feat/request #5
Feat/request #5
Conversation
todo: test method
… necessary parts of some functions.
update: doc, todo: Check for sustainable connectivity, especially fromFile, which is only tested on .html files yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for your work!
Please check my review and consider my opinions.
If you have any other opinions or counter offer, please leave comment or tell me.
include/StaticFileHandler.hpp
Outdated
# include "HttpRequest.hpp" | ||
# include "HttpResponse.hpp" | ||
|
||
# define FOLDER_PATH "./www/static" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just would like to let you know that the default directory should and will be set by the Config file in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Thanks for letting me know. Please remind me to handle this linking part later when you give the relevant PR.
src/server/HttpResponse.cpp
Outdated
} | ||
|
||
std::string getDefualtPagePath(int page_code) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The "default page" is an .html file that is default for certain directory, the filename something like "index.html", but the method is currently dealing with error page, instead.
- I think the name of the method would better with "getErrorPage".
- Typo,
getDefualtPagePath()
->getDefaultPagePath()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, now that I think about it again, Let's just go to getErrorPage
src/server/StaticFileHandler.cpp
Outdated
return ("application/octet-stream"); | ||
} | ||
|
||
bool StaticFileHandler::fileExists(const std::string path) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 얘는 어디에 쓰시려고 만드신 거 일까요? 사용처를 못 찾아서 ;;;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
staticFileHandler.cpp
line 32 if (!fileExists(filePath))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether to continue using the method or not, so I didn't write a separate doc for: the name specifies the function feature. sorry!
src/server/HttpResponse.cpp
Outdated
/// @return HttpResponse The created HttpResponse object. | ||
HttpResponse HttpResponse::fromFile(const std::string filePath) | ||
HttpResponse HttpResponse::fromFile(const std::string file_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Suggestion: How about making another constructor for HttpResponse class that take an input parameter of
file_path
to initiate, instead of using a method to make and return an instance of it.
HttpResponse::HttpResponse(const std::string file_path)
: _statusCode(200), _statusMessage("OK")
{
fromFile(file_path);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I'll update to reflect that.
Also thanks for your debug configuration with vscode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comment
src/server/StaticFileHandler.cpp
Outdated
return ("application/octet-stream"); | ||
} | ||
|
||
bool StaticFileHandler::fileExists(const std::string path) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
staticFileHandler.cpp
line 32 if (!fileExists(filePath))
src/server/HttpResponse.cpp
Outdated
/// @return HttpResponse The created HttpResponse object. | ||
HttpResponse HttpResponse::fromFile(const std::string filePath) | ||
HttpResponse HttpResponse::fromFile(const std::string file_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I'll update to reflect that.
src/server/HttpResponse.cpp
Outdated
} | ||
|
||
std::string getDefualtPagePath(int page_code) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, now that I think about it again, Let's just go to getErrorPage
include/StaticFileHandler.hpp
Outdated
# include "HttpRequest.hpp" | ||
# include "HttpResponse.hpp" | ||
|
||
# define FOLDER_PATH "./www/static" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Thanks for letting me know. Please remind me to handle this linking part later when you give the relevant PR.
src/server/StaticFileHandler.cpp
Outdated
return ("application/octet-stream"); | ||
} | ||
|
||
bool StaticFileHandler::fileExists(const std::string path) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether to continue using the method or not, so I didn't write a separate doc for: the name specifies the function feature. sorry!
A simple request/response implementation that can be used for testing.
GET
: parse the request and send the appropriate response via handleRequest.But not sure what,
and extra: I added the debug conf-available in vscode as a sample.
You can use it loosely and feel free to modify it.