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

NEW (Extension) @W-15640497@ Delta runs on SFGE - Phase 1 #130

Merged
merged 7 commits into from
Sep 20, 2024
Merged

Conversation

jag-j
Copy link
Collaborator

@jag-j jag-j commented Sep 16, 2024

No description provided.

@jag-j jag-j marked this pull request as ready for review September 16, 2024 19:48
package.json Outdated
@@ -36,6 +36,7 @@
"cross-spawn": "^7.0.3",
"glob": "^8.0.3",
"globby": "^11.0.0",
"proxyquire": "^2.1.3",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have a 3PP for this? Also, it doesn't look like this is used in production code, so should it be in devDependencies instead of dependencies?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My bad! I intended this to a devDependencies as we discussed in the tea time last week. I have moved it now.

*/
import * as fs from 'fs';

export function getDeltaRunTarget(sfgecachepath:string, savedFilesCache:Set<string>): string[] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Conceptually, it feels weird for this to be a utility method that accepts the Set, instead of having a class that encloses the Set and exposes a getDeltaRunTarget method.
But broadly speaking, I think this is fine.

src/extension.ts Outdated
const rerunFailedOnly = choice == '***Yes***';
if (rerunFailedOnly) {
const rerunChangedOnly = choice == '***Yes***';
if (rerunChangedOnly) {
// Do nothing for now. This will be implemented as part of W-15639759
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this comment need to change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@jag-j jag-j force-pushed the jj/W-15640497 branch 2 times, most recently from 1ba0d6d to ee4ecb2 Compare September 19, 2024 19:48
jfeingold35
jfeingold35 previously approved these changes Sep 19, 2024
@jag-j jag-j merged commit 6ed70e6 into dev Sep 20, 2024
7 of 8 checks passed
@jag-j jag-j deleted the jj/W-15640497 branch September 20, 2024 15:46
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.

2 participants