-
Notifications
You must be signed in to change notification settings - Fork 270
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
CORPORATION: Add new API to check if player can create corporation #1598
base: dev
Are you sure you want to change the base?
Conversation
if (checkResult !== CreatingCorporationCheckResult.Success) { | ||
helpers.log(ctx, () => checkResult); | ||
return false; | ||
} |
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.
Optional: Right as I started reviewing this, I was thinking it might be better to expose (programmatically) why you can't create a corp. Now I see you've gone and made a nice enum for handling it internally. What do you think? True/false is a much simpler API, so I'm sort of on the fence.
If you went that route, you'd want to expose the enum, which would lock us in on these strings.
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.
Returning an enum instead of true/false is a good idea. I'm all for it. However, I also have the same concern about "string-messages-in-enum-API". That downside looks too big to me.
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 if we made the enum just symbolic constants/names, and kept the string messages internal to the one place that needs them (logging)?
Edit: Just throwing ideas out here, ultimately I'd be OK with true/false, but since we've seen how hard it is to correct APIs IMO it's worth extra think time when adding them.
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 mean by "symbolic constants/names"? For an enum, we need it to be in this form:
export enum TestEnum1 {
One = "string 1",
Two = "string 2",
}
We cannot use it like these ones:
export enum TestEnum2 {
One,
Two,
}
export enum TestEnum3 {
One = 1,
Two = 2,
}
EnumHelper.ts
will throw a fit at that.
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 was thinking like
export enum CorporationCheckResult {
Success = "Success",
NoSf3OrDisabled = "NoSf3OrDisabled",
CorporationExists = "CorporationExists",
UseSeedMoneyOutsideBN3 = "UseSeedMoneyOutsideBN3",
DisabledBySoftCap = "DisabledBySoftCap",
}
I.e. the enum values are just restating the names. The values aren't any more useful than the keys here, the point is that it fits the rest of the enum architecture and we aren't worried about value stability.
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.
@Snarling We are doing an unusual thing in the API here (return an enum instead of true/false), so I want to hear your opinion.
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.
Context/side-issue: the typing of the new NS function and conversion to JSDoc
We should treat access through the top-most NS apis as a serialization boundary: return types should be symmetrically encodable and decodable to a primitive protocol like JSON or structuredClone.
There's plenty of RAM dodging use cases, backwards compatibility with netscript and some exotic messaging use cases through ports that rely on this.
An Enum is useful to convey choice internally, without the primitive being exposed. That way you can expand the primitive in scope in case you run out of choices. But that always ends up needing helpers if the enum results do get stored in a save file, end up being used for cross-script messaging or printed to a console or logs.
If you're going to use this for a user-facing API, then the use case will predominantly be to check the return value in an if or switch statement. If that's going to be a primitive then the only option left to keep it serializable is to hint it to the string subtype representation of the enum. But without the discriminated union for the enum itself,
ie.:
canCreateCorporation(selfFund: boolean): `${CreatingCorporationCheckResult}`; // ✅
canCreateCorporation(selfFund: boolean): CreatingCorporationCheckResult // ❌
canCreateCorporation(selfFund: boolean): CreatingCorporationCheckResult | `${CreatingCorporationCheckResult}` // ❌
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's a lot of discussion about whether you should/should not use Typescript's Enums. The general (outside-world) consensus seems to be "maybe not, because of (this list of issues)," but we have chosen to continue using them. We have a lot of Enums in the NS api, they are by no means rare, and they can be serialized just fine. (Although this is a more novel use of them, so I agree with catlover in asking for clarification.)
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.
Novel? I figured we were already using the toasts like this:
export type ToastVariantValues = `${ToastVariant}`;
in reference to
toast(msg: string, variant?: ToastVariantValues, duration?: number | null): void;
Is that not a good precedent?
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 "novel" part is creating an enum with keys=values.
…-if-player-can-create-corporation # Conflicts: # markdown/bitburner.corporation.createcorporation.md # src/ScriptEditor/NetscriptDefinitions.d.ts
As we discussed at #1587 (comment), this PR adds that API.