-
-
Notifications
You must be signed in to change notification settings - Fork 33
Added support for comments #99
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
base: v2
Are you sure you want to change the base?
Conversation
This comment was marked as duplicate.
This comment was marked as duplicate.
MattIPv4
left a comment
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.
Thanks for working on this!
| // Error message | ||
| if (data.message) | ||
| return `${digCmd}\n${data.message}`; | ||
|
|
||
| // No results | ||
| if (!data.answer.length) | ||
| return `${digCmd}\nNo records found${data.flags.cd | ||
| ? `\n\n${DNSSEC_DISABLED_WARNING_MESSAGE}` | ||
| : ''}`; |
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.
Do these two early returns need to handle comments?
| return `${output(finalRows)}${data.flags.cd | ||
| ? `\n${DNSSEC_DISABLED_WARNING_MESSAGE}` | ||
| : ''}`; | ||
| : ''}\n${Array.isArray(data.comment) ? `**Comments:** ${data.comment.join("; ")}` : (data.comment ? `**Comments:** ${data.comment}` : "")}`; |
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.
👀 Two thoughts here:
- Instead of the current formatting you have for comments, can we make them formatted as a quote perhaps?
- The
maxLength/finalRowslogic doesn't account for the length of the comments you're appending here.
| type, | ||
| `@${provider.dig}`, | ||
| '+noall', | ||
| '+answer', |
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 think we also need +comments in the equivalent dig command now?
| `@${provider.dig}`, | ||
| '+noall', | ||
| '+answer', | ||
| options.short ? '+short' : null, |
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.
In the spirit of the +short option from dig itself, I wonder if we shouldn't show the comments when short is requested?
|
@mathew2103 any update on this? |
|
Got a bit tied up with uni exams, I will make changes as per the review this week.. Apologies |


Fixes #77