-
Notifications
You must be signed in to change notification settings - Fork 289
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
CODINGCONTRACT: Add support for other answer formats #1892
base: dev
Are you sure you want to change the base?
Conversation
I spent about 1 hour now, testing "most" contracts for backwards-compatibility with my own cct script. I would really appreciate others testing their scripts too, since mine doesn't cover all contracts. These are the contracts, I would say, that are most affected by the string conversion and taking the format directly:
|
I'm working on making the types available to the player right now. In the definitions, it is suggested that the data is provided similar to how the declare enum CodingContractName {
FindLargestPrimeFactor = "Find Largest Prime Factor",
...
}
// Each contract has its data and answer types specified here. The player can use this however they like.
export type CodingContractSchema = {
"Find Largest Prime Factor": [number, number],
"Shortest Path in a Grid": [number[][], string],
"Square Root": [bigint, string],
[CodingContractName.FindLargestPrimeFactor]: [number, number] // Maybe this is safer?
}
// This would be the new way the contract data is returned, allowing for type-narrowing based on "type".
export type CodingContractData = { [T in keyof CodingContractSchema]: {
type: `${T}`,
data: CodingContractSchema[T][0]
} }[keyof CodingContractSchema];
/* This results in this type signature:
type CodingContractData =
| { type: "Find Largest Prime Factor"; data: number; }
| { type: "Shortest Path in a Grid"; data: number[][]; }
| { type: "Square Root"; data: bigint; }
*/ The new way of returning the contract data is a API break, so maybe the optional typing is preferred? (see this playground example).
There are quite a few options here:
This is something I really don't want to decide on my own here. IMO it isn't worth it for |
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 don't understand why you want to change the API of getData
. The type of contracts is already exposed via ns.codingcontract.getType()
; the function is literally named for this purpose. What gain is there by additionally jamming it into getData
?
Edit: I could see there being a slightly better alternate world where ns.codingcontract.getContract()
returned a single object with all the data pertinent to a CCT, and we didn't have multiple API functions to do the different jobs, but that's not the world we are in.
Changing the way the data is provided to the user can give type-safety, even to non-TS users. Depending on the I agree, it would make more sense for that function to be called
That's why I'd also like |
I'm all for
I think I'd be open to adding a I don't think |
Since all of the contracts types are in the definitions now, the internals can be somewhat type-safe too. At least the solver definitions. |
Someone should really test their cct script on this. In theory, it should be 100% backwards-compatible. |
I tested this script on the current 2.7.1dev and this PR and it works on both versions without any errors. Well, except for contract filenames allowing for duplicates, which is an unrelated bug. I will fix that in a separate PR when this is merged. |
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 internal typesafety changes are great. I've only got one (somewhat significant) concern about the API
@@ -18,11 +19,20 @@ export function NetscriptCodingContract(): InternalAPI<ICodingContract> { | |||
}; | |||
|
|||
return { | |||
attempt: (ctx) => (answer, _filename, _hostname?) => { | |||
attempt: (ctx) => (answer: unknown, _filename: unknown, _hostname?: unknown, _type?: unknown) => { |
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.
You don't have to explicitly type these, all parameters are automatically unknown through a process I don't fully understand/recall.
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.
Leaving out the explicit types actually results in an error. I'm not sure either, why this is the case. For getContract
, I can remove them safely though.
* @param filename - Filename of the contract. | ||
* @param host - Hostname of the server containing the contract. Optional. Defaults to current server if not | ||
* provided. | ||
* @returns A reward description string on success, or an empty string on failure. | ||
*/ | ||
attempt(answer: string | number | any[], filename: string, host?: string): string; | ||
attempt(answer: any, filename: string, host?: string): string; | ||
attempt<T extends CodingContractName | `${CodingContractName}`>( |
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 a big fan of having two versions of the function, but I think trying to collapse their signatures would be even worse.
Note that the TSDoc here is only applying to the first version; the second one is undocumented. It pains me to duplicate all the documentation for the other version, but I think you're gonna have to. The docs about the forth parameter should only go on the second override ofc. Also, you should mention that it's only useful for people writing native Typescript code.
Honestly, the whole thing leaves a very sour taste in my mouth, because it feels awkward and shoehorned in just for TS, and even then, not really quite the right design. The most "natural" design, where all type info would flow automatically and clearly, would be getContract
returning a class that had attempt
on it. Unfortunately, the way the ns object works makes this tricky, although not impossible.
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.
It is possible to collapse the signature. It's not that clean, but it could work.
I don't think copying the documentation would be a good idea. The documentation should actually work on all overloads here, just not when filling in the arguments, which is pretty important.
A function on getContract
would be a good option, since it avoids all of this. We would run in the same situation as with the port handles. Since the function can only be used up to the maximum tries for any given contract, it should be fine though? Handling the script death would still be a weird quirk.
From what I can tell, it's either merging the signatures or a function on getContract
. I'm fine with both suggestions tbh.
If we choose to add a callback there, I would tend to remove the typings on attempt
and getData
at the same time. I don't like adding it on one, but not the other.
The new signature would be something like this: attempt<T extends CodingContractName | ´${CodingContractName}´ | "" = "">(answer: CodingContractAnswer<T>, filename: string, host?: string, type?: T): string;
. It's not that bad IMO, but I could see how it confuses some players.
Coding contracts are inherently not type-safe. This PR tries to combat this a bit, by making these changes:
attempt
accept not only strings, but also the specific type the contract works with, likenumber[]
or more specific formatsgetData
andattempt