Skip to content

Move common code + class definition updates #36120

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

scbedd
Copy link
Member

@scbedd scbedd commented Jul 22, 2025

Addresses:

  • Clean up class definition per this comment
  • Move some isManagement or isReleaseBranch to SpecModel or common helpers.
    • Utilize changed-files utilities instead of redetecting in IsDataPlanePR and isManagementPR
    • Moved isReleaseBranch to common code.

Copy link

openapi-pipeline-app bot commented Jul 22, 2025

Next Steps to Merge

Next steps that must be taken to merge this PR:
  • ❌ The required check named Protected Files has failed. Refer to the check in the PR's 'Checks' tab for details on how to fix it and consult the aka.ms/ci-fix guide

Copy link

PR validation pipeline started successfully. If there is ApiView generated, it will be updated in this comment.

@@ -20,7 +20,8 @@
"./swagger": "./src/swagger.js",
"./tag": "./src/tag.js",
"./simple-git": "./src/simple-git.js",
"./test/examples": "./test/examples.js"
"./test/examples": "./test/examples.js",
Copy link
Member Author

Choose a reason for hiding this comment

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

I did notice this looks odd. Was this intentional? If so I'll continue leaving this alone.

@@ -0,0 +1,9 @@
/**
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't really see a good place to put something branch related like this. I DID consider sdk-types.js, but that seemed like it was type related specifically so I didn't put it there.

@scbedd scbedd moved this from 🤔 Triage to 🔬 Dev in PR in Azure SDK EngSys 🍕 Jul 23, 2025
* The name of the label.
* @type {string}
*/
name;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
name;
#name;

* Is the label currently present on the pull request?
* @type {boolean}
*/
present;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
present;
#present;

* label construction logic.
* @type {boolean | undefined}
*/
shouldBePresent;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
shouldBePresent;
#shouldBePresent;

Copy link
Member

@mikeharder mikeharder left a comment

Choose a reason for hiding this comment

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

Approved pending rename of private fields

@scbedd
Copy link
Member Author

scbedd commented Jul 23, 2025

Approved pending rename of private fields

They're not private. They need to be accessible as various code points interact with those values directly. I'll check how hard it will be to refactor so they actually are private tho.

@mikeharder
Copy link
Member

mikeharder commented Jul 23, 2025

Approved pending rename of private fields

They're not private. They need to be accessible as various code points interact with those values directly. I'll check how hard it will be to refactor so they actually are private tho.

If they only need to be read (not written) by callers, you can add a getter:

/**
* @returns {string} absolute path
*/
get path() {
return this.#path;
}

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

Successfully merging this pull request may close these issues.

2 participants