Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
CORPORATION: Add new API to check if player can create corporation #1598
Changes from all commits
d948a7b
45fa7a3
86aa108
513548a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
We cannot use it like these ones:
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
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.:
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:
in reference to
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.