-
Notifications
You must be signed in to change notification settings - Fork 106
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
Feature/dropfile improvements #521
base: master
Are you sure you want to change the base?
Conversation
let formatObj = getPredefinedMCIFormatObject(this.client, text); | ||
|
||
const additionalFormatObj = { | ||
'getSysopFirstName': this.getSysopFirstName(), |
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.
small nit here, remove the get
prefix to match the rest of how the system treats these types of variables
'getSysopLastName': this.getSysopLastName(), | ||
'getUserFirstName': this.getUserFirstName(), | ||
'getUserLastName': this.getUserLastName(), | ||
'getUserTotalDownloadK': this.getUserTotalDownloadK(), |
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 {userTotalDownloadBytes!toKilobytes}
(by resolving some TODO's in string_format.js
would work here and be more flexible?
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.
So this one and the date format I had thought about, but there is a bit of a problem with when these methods are called... it looks like the methods run first and return a string, then the formatting runs on them. So for these it would actually have to parse as a string to format, which is a bit ugly. Maybe a better way would be to have these return an object with a toString instead, then we can format them as we like, with the default behaving as it does now? Just a thought.
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'll take a look at this maybe tomorrow night. I think in string_format.js
it can be expanded to allow some Date/Time raw formats of some sort and via transformers:
try {
value = getValue(obj, objPath);
if (transformer) {
// 'value' can be a obj here IIRC
value = transformValue(transformer, value);
}
tokens = tokenizeFormatSpec(formatSpec || '');
// could allow non-transformers to handle D/T here too... maybe
if (_.isNumber(value)) {
out += formatNumber(value, tokens);
} else {
out += formatString(value, tokens);
}
} catch (e) {
if (e instanceof KeyError) {
out += match[0]; // preserve full thing
} else if (e instanceof ValueError) {
out += value.toString();
}
}
'getUserLastName': this.getUserLastName(), | ||
'getUserTotalDownloadK': this.getUserTotalDownloadK(), | ||
'getUserTotalUploadK': this.getUserTotalUploadK(), | ||
'getCurrentDateMMDDYY': this.getCurrentDateMMDDYY(), |
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.
We might be able to do it here as well, something like {currentDate!dateMmDdYy}
or something, with some other common formats.
// Read the directory containing the dropfile formats, and return undefined if we don't have the format | ||
const fileName = this.fileName; | ||
if (!fileName) { | ||
Log.info({fileType: this.fileType}, 'Dropfile format not supported.'); |
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.
Should that be a warn?
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.
Agreed I'll change these to warns
@@ -210,7 +212,7 @@ function formatNumberHelper(n, precision, type) { | |||
function formatNumber(value, tokens) { | |||
const fill = tokens.fill || (tokens['0'] ? '0' : ' '); | |||
const align = tokens.align || (tokens['0'] ? '=' : '>'); | |||
const width = Number(tokens.width); | |||
const width = Number(tokens.width);value.replace(/\x1b\[[0-9;]*m/g, ''); |
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.
formatting nit
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.
Sorry I'll clean that up
@@ -0,0 +1,36 @@ | |||
{UR} |
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.
Very minor, but can we call these "dropfile_templates" to match the 'template' naming elsewhere?
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.
Will do
Any thoughts on how the binary formats would work here? Not exactly pressing of course... I'll pull this down and start poking. At glance it looks 100% backwards compatible, which is 👍 |
@@ -299,6 +301,7 @@ const transformers = { | |||
styleSmallI: s => stylizeString(s, 'small i'), | |||
styleMixed: s => stylizeString(s, 'mixed'), | |||
styleL33t: s => stylizeString(s, 'l33t'), | |||
sanitized: s => stylizeString(s, 'sanitized'), |
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.
We'll want to get this guy documented
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.
Should have done that already 🙂 will do
@@ -124,6 +124,12 @@ const PREDEFINED_MCI_GENERATORS = { | |||
UN: function userName(client) { | |||
return client.user.username; | |||
}, | |||
UZ: function sanitizedUserName(client) { |
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.
Nice! We'll need to get these in the MCI docs
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.
Yeah I actually probably need to do a few more of these too and think about the split - I.e I could also add first name / last name ones etc among others. Probably needs some more thinking on what is appropriate here
1 | ||
0 | ||
99999 | ||
C:\DATA |
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 wonder if we'll need to allow these to be configured. I imagine a WWIV door running under e.g. DOSEMU, where D:\SOMETHING\DATA
is where it wants to perform I/O. I'm not sure though...
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.
They absolutely will need to be - I didn't have anything to set them to yet, so left them for now. Before this gets merged though hopefully will be able to set them to what we need for either existing door system or the new one. I just didn't want to wait for that for you to get a chance to check out the overall direction etc
I left room for specific implementations that don't use the templates - they would return a method the same as they did before for the implementation. I haven't put any in yet though - I know I have the format for at least one binary dropfile format though I can go ahead and add that to work as an example too |
|
||
// :TODO: fix time remaining | ||
// :TODO: fix default protocol -- user prop: transfer_protocol | ||
return iconv.encode( |
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.
One thing I think we're losing here is the self-commenting dropfile formats.
@@ -0,0 +1,52 @@ | |||
COM1: |
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.
What do you think about supporting comments in the templates so we can self-doc? I think // anything after
would work for all of them
Updated to 8 dropfile formats and changed to use a template file.
Note: additional testing and changes are most likely needed for the formats for both the current door support as well as the new door system, I wanted to get this in now though to get the conversation started about changes needed, etc.