Skip to content
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

API: Add new feature "ns.${Api}.isEligible" functions for Bladeburner and Stanek APIs. #1911

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

ivanrodgon
Copy link

Pull Request Description

Summary

This pull request introduces the isEligible function for the Stanek and Bladeburner APIs, aligning with the discussion in issue #1632. The goal is to improve API integrity by ensuring proper access validation for players interacting with these features.

Changes

  1. Implemented isEligible Function

    • A new utility function, isEligible, has been created to determine if a player meets the requirements to access the Stanek and Bladeburner APIs.
  2. Integrated Eligibility Checks

    • Added the isEligible validation to the corresponding methods in the Stanek API.
    • Added the isEligible validation to the corresponding methods in the Bladeburner API.
  3. Code Refactoring and Cleanup

    • Minor adjustments were made to streamline the implementation and improve readability in the affected files.
    • Ensured consistency in error messages when a player lacks access.

Related Issue

This pull request addresses Issue #1632.

Copy link
Contributor

@catloversg catloversg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I'll use the phrase "activate X" as an "alias" for creating/joining/accepting gang/corporation/Bladeburner/Stanek.

From a player's perspective, they usually want to know:

  • Can I activate a mechanic? For example: Can I activate X? This is useful because some activation APIs throw errors when I violate non-obvious constraints (e.g., ns.corporation.createCorporation throws in BN8). [1]
  • Can I use NS APIs to manage a mechanic? For example: Did I activate X? If yes, can I call APIs of X to manage it? [2]

[1]: This is the kind of isEligible APIs that you want to add. For corporation, we have ns.corporation.canCreateCorporation.
[2]: There are 2 things here:

  • Did I activate X?: We have ns.gang.inGang, ns.corporation.hasCorporation, ns.bladeburner.inBladeburner. There is no equivalent API for Stanek. [3]
  • can I call APIs of X to manage it?: This is where things get messy. Generally, if you activated X, you would be able to call X's APIs, but there are still exceptions:
    • Bladeburner: No SF7 = No API [4]
    • Corporation: No SF3.3 = Have to buy Office API and Warehouse API before using them.

Due to [3], I suggest adding ns.stanek.hasGift instead of ns.stanek.checkStanekAPIAccess. It will be more consistent with old APIs.

Due to [4], I think checkBladeburnerAPIAccess is unnecessary. The player can call inBladeburner and check SF7 by using ns.singularity.getOwnedSourceFiles or ns.getResetInfo().ownedSF. The requirement of SF7 is obvious. It's mentioned in both documentation and the BN selection screen. If we merge #1407, the requirement of SF7 will be gone. In that case, calling inBladeburner is enough.

Another thing to discuss: Is isEligible a good name? IMO, it's better to explicitly call it canActivateX. For example, canJoinBladeburner, canAcceptGift, etc. Without looking at the documentation, isEligible may mean "Can I activate X" or "Can I manage X via APIs?". Having only one API name for all mechanics may be easier for players to find them, but I don't think there is enough benefit to make the trade-off.

@@ -402,15 +406,15 @@ export class Bladeburner implements OperationTeam {
this.postToConsole("Automation: " + (this.automateEnabled ? "enabled" : "disabled"));
this.postToConsole(
"When your stamina drops to " +
formatNumberNoSuffix(this.automateThreshLow, 0) +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to run npm run format before committing changes.

@@ -4628,12 +4646,12 @@ export interface Go {
*
* For example, a 5x5 board might look like this:
*
[<br/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These space characters are intentional. If your IDE makes these changes, you have to revert them before committing.

@@ -82,7 +87,7 @@ export function NetscriptBladeburner(): InternalAPI<INetscriptBladeburner> {
return { name: blackOp.name, rank: blackOp.reqdRank };
},
getBlackOpRank: (ctx) => (_blackOpName) => {
checkBladeburnerAccess(ctx);
getBladeburner(ctx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming here is weird. checkSomething() clearly means that it will perform a check, which is exactly what it's doing. When you use getBladeburner, that name makes it look like we use a getter, but we don't care about the returned value. I think you are trying to refactor and expose checkBladeburnerAPIAccess as an API. IMO, checkBladeburnerAPIAccess is unnecessary. More about this later.

@d0sboots
Copy link
Collaborator

I agree with Catlover's comments. In particular, there is also a subtle issue with having functions named the same thing, related to the static-RAM check. It's not so much an issue for zero-cost functions, but it still becomes a complete non-issue if functions have unique names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants