-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat: use baseUrl to display full URL #97
Conversation
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.
Thank you for PR. I will review on weekend.
2bff60f
to
96b3531
Compare
Just forgot to update |
src/logger/response.ts
Outdated
const buildConfig = assembleBuildConfig(config); | ||
|
||
const stringBuilder = new StringBuilder(buildConfig); | ||
const log = stringBuilder | ||
.makeLogTypeWithPrefix('Response') | ||
.makeDateFormat(new Date()) | ||
.makeMethod(method) | ||
.makeUrl(url) | ||
.makeUrl(stringBuilder.combineURLs(baseURL, url)) |
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.
MakeUrl is an abstract method of logic that manages url. I think the combine URLs business logic should be inside makeUrl.
How about .makeUrl(url, baseUrl)
?
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.
Sure! done :)
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.
Good. I will test and merge. Thank you.
96b3531
to
5f1ba4e
Compare
5f1ba4e
to
841b9ed
Compare
This fixes #60 and takes into consideration the
baseURL
parameter of an axios instance.