Skip to content

@W-21557588 2pp review#7

Merged
joroscoSF merged 4 commits intomainfrom
ew/review
Mar 25, 2026
Merged

@W-21557588 2pp review#7
joroscoSF merged 4 commits intomainfrom
ew/review

Conversation

@iowillhoit
Copy link
Contributor

@iowillhoit iowillhoit commented Mar 25, 2026 •
edited
Loading

This PR standardizes a lot of plugin structure. I'll add some inline comments
@W-21557588@

@git2gus
Copy link

git2gus bot commented Mar 25, 2026

Git2Gus App is installed but the .git2gus/config.json doesn't exist.

iowillhoit
iowillhoit commented Mar 25, 2026
View reviewed changes
iowillhoit
iowillhoit commented Mar 25, 2026
View reviewed changes
src/base/deployBase.ts
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
Copy link
Contributor Author

@iowillhoit iowillhoit Mar 25, 2026

Choose a reason for hiding this comment

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

These license headers were added with the yarn fix-license script that was added via dev-scripts

Copy link
Contributor

@joroscoSF joroscoSF Mar 25, 2026

Choose a reason for hiding this comment

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

good to know

iowillhoit
iowillhoit commented Mar 25, 2026
View reviewed changes
.nycrc
"functions": 75,
"branches": 75
} No newline at end of file
"extends": "@salesforce/dev-config/nyc"
Copy link
Contributor Author

@iowillhoit iowillhoit Mar 25, 2026

Choose a reason for hiding this comment

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

This is default config. Unless there is a good reason to change it, I would leave it

Copy link
Contributor

@joroscoSF joroscoSF Mar 25, 2026

Choose a reason for hiding this comment

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

sounds good

iowillhoit
iowillhoit commented Mar 25, 2026
View reviewed changes
iowillhoit
iowillhoit commented Mar 25, 2026
View reviewed changes
package.json
"eslint-plugin-sf-plugin": "^1.18.6",
"@oclif/plugin-command-snapshot": "^5.2.3",
"@salesforce/cli-plugins-testkit": "^5.3.20",
"@salesforce/dev-scripts": "^11.0.4",
Copy link
Contributor Author

@iowillhoit iowillhoit Mar 25, 2026

Choose a reason for hiding this comment

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

I bumped dev-scripts to 11.0.4 and ran yarn. It bumped all of the other dependencies and devDependencies (as designed)

Copy link
Contributor

@joroscoSF joroscoSF Mar 25, 2026

Choose a reason for hiding this comment

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

I noticed dependabot complaining and I'd imagine this will squelch most of that. Was wondering and now I know.

iowillhoit
iowillhoit commented Mar 25, 2026
View reviewed changes
package.json
"clean-all": "sf-clean all",
"compile": "wireit",
"docs": "sf-docs",
"fix-license": "eslint src test --fix --rule \"header/header: [2]\"",
Copy link
Contributor Author

@iowillhoit iowillhoit Mar 25, 2026

Choose a reason for hiding this comment

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

Added by dev-scripts. Running this fixes/adds the license headers

Copy link
Contributor

@joroscoSF joroscoSF Mar 25, 2026

Choose a reason for hiding this comment

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

The more you know

iowillhoit
iowillhoit commented Mar 25, 2026
View reviewed changes
package.json
"format": "wireit",
"link-check": "wireit",
"lint": "wireit",
"postinstall": "node -e \"const r=require('child_process').spawnSync('./node_modules/.bin/husky',['install'],{stdio:'inherit'});if(r.error&&r.error.code!=='ENOENT')process.exit(1);\"",
Copy link
Contributor Author

@iowillhoit iowillhoit Mar 25, 2026

Choose a reason for hiding this comment

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

Not sure where this came from? I deleted it in favor of "prepare": "sf-install", <-- this is what hooks into dev-scripts to make a bunch of changes to keep everything in sync

iowillhoit
iowillhoit commented Mar 25, 2026
View reviewed changes
package.json
"prepare": "sf-install",
"test": "wireit",
"test:nuts": "node scripts/run-nuts.cjs",
"test:nuts": "nyc mocha \"**/*.nut.ts\" --slow 4500 --timeout 600000 --parallel",
Copy link
Contributor Author

@iowillhoit iowillhoit Mar 25, 2026

Choose a reason for hiding this comment

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

Deleted this "do NUT tests exists script" and just commented out the lines in the tests.yml Github Action above. If you feel strongly about this, you can add it back

Copy link
Contributor

@joroscoSF joroscoSF Mar 25, 2026

Choose a reason for hiding this comment

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

I feel strongly about learning the proper/accepted way of doing things on my Typescript voyage.

@iowillhoit
Copy link
Contributor Author

iowillhoit commented Mar 25, 2026

A few more not-super-urgent things:

  • Deploy --description uses -d, and Scan --dry-run also uses -d. They won't collided, but it could confuse users. If it were me, I would remove the -d short-char on --dry-run.
  • There are a lot of "check the python env" messages (e.g. info.pythonFound) repeated over and over. You are not limited to a single message file that you import into a command. So, you could move those repeated messages to a shared messages file (i.e. envInfo.md), import that file where needed, then pass that along when calling checkEnvironment()
  • No tests for run?
  • I think the way that shared base classes are being used could be cleaned up, can revisit later

joroscoSF
joroscoSF approved these changes Mar 25, 2026
View reviewed changes
src/base/deployBase.ts
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
Copy link
Contributor

@joroscoSF joroscoSF Mar 25, 2026

Choose a reason for hiding this comment

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

good to know

.nycrc
"functions": 75,
"branches": 75
} No newline at end of file
"extends": "@salesforce/dev-config/nyc"
Copy link
Contributor

@joroscoSF joroscoSF Mar 25, 2026

Choose a reason for hiding this comment

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

sounds good

package.json
"eslint-plugin-sf-plugin": "^1.18.6",
"@oclif/plugin-command-snapshot": "^5.2.3",
"@salesforce/cli-plugins-testkit": "^5.3.20",
"@salesforce/dev-scripts": "^11.0.4",
Copy link
Contributor

@joroscoSF joroscoSF Mar 25, 2026

Choose a reason for hiding this comment

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

I noticed dependabot complaining and I'd imagine this will squelch most of that. Was wondering and now I know.

package.json
"clean-all": "sf-clean all",
"compile": "wireit",
"docs": "sf-docs",
"fix-license": "eslint src test --fix --rule \"header/header: [2]\"",
Copy link
Contributor

@joroscoSF joroscoSF Mar 25, 2026

Choose a reason for hiding this comment

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

The more you know

package.json
"prepare": "sf-install",
"test": "wireit",
"test:nuts": "node scripts/run-nuts.cjs",
"test:nuts": "nyc mocha \"**/*.nut.ts\" --slow 4500 --timeout 600000 --parallel",
Copy link
Contributor

@joroscoSF joroscoSF Mar 25, 2026

Choose a reason for hiding this comment

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

I feel strongly about learning the proper/accepted way of doing things on my Typescript voyage.

@joroscoSF joroscoSF merged commit abe2c73 into main Mar 25, 2026
12 checks passed
@joroscoSF joroscoSF deleted the ew/review branch March 25, 2026 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joroscoSF joroscoSF joroscoSF approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@iowillhoit @joroscoSF