@@ -75,13 +75,13 @@ export type FileInfo = {
7575 path : string ,
7676 kind : FileKind ,
7777 suspect ?: string , // reason for a file being "package-meta" rather than "package-meta-ok"
78- suggestion ?: Suggestion ,
78+ suggestion ?: Suggestion , // The differences from the required form, as GitHub suggestions
7979} ;
8080
8181export interface Suggestion {
8282 readonly startLine : number ;
83- readonly line : number ;
84- readonly suggestion : string ;
83+ readonly endLine : number ;
84+ readonly body : string ;
8585}
8686
8787export type ReviewInfo = {
@@ -227,9 +227,17 @@ export async function deriveStateForPR(
227227 const lastBlessing = getLastMaintainerBlessingDate ( prInfo . timelineItems ) ;
228228 const reopenedDate = getReopenedDate ( prInfo . timelineItems ) ;
229229
230+ const refs = {
231+ head : headCommit . oid ,
232+ master : "master" ,
233+ latestSuggestions : prInfo . reviews ?. nodes ?. reduce ( ( latest , review ) =>
234+ review && ! authorNotBot ( review ) && (
235+ ! latest ?. submittedAt || review . submittedAt && new Date ( review . submittedAt ) > new Date ( latest . submittedAt ) )
236+ ? review : latest , null ) ?. commit ?. oid ,
237+ } ;
230238 const pkgInfoEtc = await getPackageInfosEtc (
231239 noNullish ( prInfo . files ?. nodes ) . map ( f => f . path ) . sort ( ) ,
232- headCommit . oid , fetchFile , async name => await getDownloads ( name , lastPushDate ) ) ;
240+ refs , fetchFile , async name => await getDownloads ( name , lastPushDate ) ) ;
233241 if ( pkgInfoEtc instanceof Error ) return botError ( prInfo . number , pkgInfoEtc . message ) ;
234242 const { pkgInfo, popularityLevel } = pkgInfoEtc ;
235243 if ( ! pkgInfo . some ( p => p . name ) ) return botNoPackages ( prInfo . number ) ;
@@ -326,11 +334,11 @@ function getLastMaintainerBlessingDate(timelineItems: PR_repository_pullRequest_
326334}
327335
328336async function getPackageInfosEtc (
329- paths : string [ ] , headId : string , fetchFile : typeof defaultFetchFile , getDownloads : typeof getMonthlyDownloadCount
337+ paths : string [ ] , refs : { [ ref : string ] : string } , fetchFile : typeof defaultFetchFile , getDownloads : typeof getMonthlyDownloadCount
330338) : Promise < { pkgInfo : PackageInfo [ ] , popularityLevel : PopularityLevel } | Error > {
331339 const infos = new Map < string | null , FileInfo [ ] > ( ) ;
332340 for ( const path of paths ) {
333- const [ pkg , fileInfo ] = await categorizeFile ( path , async ( oid : string = headId ) => fetchFile ( `${ oid } ${ path } ) ) ;
341+ const [ pkg , fileInfo ] = await categorizeFile ( path , async ( ref : string ) => fetchFile ( `${ refs [ ref ] } ${ path } ) ) ;
334342 if ( ! infos . has ( pkg ) ) infos . set ( pkg , [ ] ) ;
335343 infos . get ( pkg ) ! . push ( fileInfo ) ;
336344 }
@@ -341,7 +349,7 @@ async function getPackageInfosEtc(
341349 if ( oldOwners instanceof Error ) return oldOwners ;
342350 const newOwners0 = ! name ? null
343351 : ! paths . includes ( `types/${ name } ) ? oldOwners
344- : await getOwnersOfPackage ( name , headId , fetchFile ) ;
352+ : await getOwnersOfPackage ( name , refs . head , fetchFile ) ;
345353 // A header error is still an add/edit whereas a missing file is
346354 // delete, hence newOwners0 here
347355 const kind = ! name ? "edit" : ! oldOwners ? "add" : ! newOwners0 ? "delete" : "edit" ;
@@ -365,7 +373,7 @@ async function getPackageInfosEtc(
365373 return { pkgInfo : result , popularityLevel : downloadsToPopularityLevel ( maxDownloads ) } ;
366374}
367375
368- async function categorizeFile ( path : string , contents : ( oid ? : string ) => Promise < string | undefined > ) : Promise < [ string | null , FileInfo ] > {
376+ async function categorizeFile ( path : string , contents : ( ref : string ) => Promise < string | undefined > ) : Promise < [ string | null , FileInfo ] > {
369377 // https://regex101.com/r/eFvtrz/1
370378 const match = / ^ t y p e s \/ ( .* ?) \/ .* ?[ ^ \/ ] (?: \. ( d \. t s | t s x ? | m d ) ) ? $ / . exec ( path ) ;
371379 if ( ! match ) return [ null , { path, kind : "infrastructure" } ] ;
@@ -381,25 +389,18 @@ async function categorizeFile(path: string, contents: (oid?: string) => Promise<
381389}
382390
383391interface ConfigSuspicious {
384- ( path : string , getContents : ( oid ? : string ) => Promise < string | undefined > ) : Promise < { suspect : string , sugestion ?: Suggestion } | undefined > ;
385- [ basename : string ] : ( text : string , oldText ?: string ) => { suspect : string , suggestion ?: Suggestion } | undefined ;
392+ ( path : string , getContents : ( ref : string ) => Promise < string | undefined > ) : Promise < { suspect : string , sugestion ?: Suggestion } | undefined > ;
393+ [ basename : string ] : ( text : string , getContents : ( ref : string ) => Promise < string | undefined > ) => Promise < { suspect : string , suggestion ?: Suggestion } | undefined > ;
386394}
387395const configSuspicious = < ConfigSuspicious > ( async ( path , getContents ) => {
388396 const basename = path . replace ( / .* \/ / , "" ) ;
389397 if ( ! ( basename in configSuspicious ) ) return { suspect : `edited` } ;
390- const text = await getContents ( ) ;
398+ const text = await getContents ( "head" ) ;
391399 if ( text === undefined ) return { suspect : `couldn't fetch contents` } ;
392400 const tester = configSuspicious [ basename ] ;
393- let suspect ;
394- if ( tester . length === 1 ) {
395- suspect = tester ( text ) ;
396- } else {
397- const oldText = await getContents ( "master" ) ;
398- suspect = tester ( text , oldText ) ;
399- }
400- return suspect ;
401+ return tester ( text , getContents ) ;
401402} ) ;
402- configSuspicious [ "OTHER_FILES.txt" ] = contents =>
403+ configSuspicious [ "OTHER_FILES.txt" ] = async contents =>
403404 // not empty
404405 ( contents . length === 0 ) ? { suspect : "empty" }
405406 : undefined ;
@@ -433,51 +434,73 @@ configSuspicious["tsconfig.json"] = makeJsonCheckerFromCore(
433434// to it, ignoring some keys (JSON Patch paths). The ignored properties are in most cases checked
434435// elsewhere (dtslint), and in some cases they are irrelevant.
435436function makeJsonCheckerFromCore ( requiredForm : any , ignoredKeys : string [ ] , requiredFormUrl ?: string ) {
436- return ( contents : string , oldText ?: string ) => {
437- const theRequiredForm = requiredFormUrl
438- ? `[the required form](${ requiredFormUrl }
439- : "the required form" ;
440- const diffFromReq = ( json : any ) => {
441- jsonDiff . applyPatch ( json , ignoredKeys . map ( path => ( { op : "remove" , path } ) ) ) ;
442- return jsonDiff . compare ( requiredForm , json ) ;
443- } ;
444- let newJson ;
445- try { newJson = JSON . parse ( contents ) ; } catch ( e ) { return { suspect : "couldn't parse json" } ; }
446- const suggestion = jsonDiff . deepClone ( newJson ) ;
447- const newDiff = diffFromReq ( newJson ) ;
448- if ( newDiff . length === 0 ) return undefined ;
437+ return async ( contents : string , getContents : ( ref : string ) => Promise < string | undefined > ) => {
438+ let suggestion : any ;
439+ try { suggestion = JSON . parse ( contents ) ; } catch ( e ) { return { suspect : "couldn't parse json" } ; }
440+ const newJson = jsonDiff . deepClone ( suggestion ) ;
441+ jsonDiff . applyPatch ( newJson , ignoredKeys . map ( path => ( { op : "remove" , path } ) ) ) ;
449442 const towardsIt = jsonDiff . deepClone ( requiredForm ) ;
450- if ( oldText ) {
443+ // Getting closer to the required form relative to master isn't
444+ // suspect
445+ const vsMaster = await ignoreExistingDiffs ( "master" ) ;
446+ if ( ! vsMaster ) return undefined ;
447+ if ( vsMaster . done ) return { suspect : vsMaster . suspect } ;
448+ // whereas getting closer relative to existing suggestions makes
449+ // no further suggestions
450+ if ( ! await ignoreExistingDiffs ( "latestSuggestions" ) ) return { suspect : vsMaster . suspect } ;
451+ jsonDiff . applyPatch ( suggestion , jsonDiff . compare ( newJson , towardsIt ) ) ;
452+ return {
453+ suspect : vsMaster . suspect ,
454+ suggestion : makeJsonSuggestion ( ) ,
455+ } ;
456+
457+ // Apply any preexisting diffs to towardsIt
458+ async function ignoreExistingDiffs ( ref : string ) {
459+ const theRequiredForm = requiredFormUrl
460+ ? `[the required form](${ requiredFormUrl }
461+ : "the required form" ;
462+ const diffFromReq = ( json : any ) => jsonDiff . compare ( towardsIt , json ) ;
463+ const newDiff = diffFromReq ( newJson ) ;
464+ if ( newDiff . length === 0 ) return undefined ;
465+ const oldText = await getContents ( ref ) ;
466+ if ( ! oldText ) return { suspect : `not ${ theRequiredForm } } ;
451467 let oldJson ;
452- try { oldJson = JSON . parse ( oldText ) ; } catch ( e ) { return { suspect : "couldn't parse json" } ; }
468+ try { oldJson = JSON . parse ( oldText ) ; } catch ( e ) { return { done : true , suspect : "couldn't parse json" } ; }
469+ jsonDiff . applyPatch ( oldJson , ignoredKeys . map ( path => ( { op : "remove" , path } ) ) ) ;
453470 const oldDiff = diffFromReq ( oldJson ) ;
454471 const notRemove = jsonDiff . compare ( oldDiff , newDiff ) . filter ( ( { op } ) => op !== "remove" ) ;
455472 if ( notRemove . length === 0 ) return undefined ;
456473 jsonDiff . applyPatch ( newDiff , notRemove . map ( ( { path } ) => ( { op : "remove" , path } ) ) ) ;
457474 jsonDiff . applyPatch ( towardsIt , newDiff . filter ( ( { op } ) => op ) ) ;
475+ return { suspect : `not ${ theRequiredForm } } ;
458476 }
459- jsonDiff . applyPatch ( suggestion , jsonDiff . compare ( newJson , towardsIt ) ) ;
477+
460478 // Suggest the different lines to the author
461- const suggestionLines = (
462- Object . keys ( suggestion ) . length <= 1
463- ? prettier . format ( JSON . stringify ( suggestion ) , { tabWidth : 4 , filepath : ".json" } )
464- : JSON . stringify ( suggestion , undefined , 4 ) + "\n"
465- ) . split ( / ^ / m) ;
466- const lines = contents . split ( / ^ / m) ;
467- let i = 0 ;
468- while ( suggestionLines [ i ] . trim ( ) === lines [ i ] . trim ( ) ) i ++ ;
469- let j = 1 ;
470- while ( suggestionLines [ suggestionLines . length - j ] . trim ( ) === lines [ lines . length - j ] . trim ( ) ) j ++ ;
471- return {
472- suspect : oldText
473- ? `not ${ theRequiredForm }
474- : `not ${ theRequiredForm } ,
475- suggestion : {
476- startLine : i + 1 ,
477- line : lines . length - j + 1 ,
478- suggestion : suggestionLines . slice ( i , 1 - j || undefined ) . join ( "" ) ,
479- } ,
480- } ;
479+ function makeJsonSuggestion ( ) {
480+ const suggestionLines = (
481+ Object . keys ( suggestion ) . length <= 1
482+ ? prettier . format ( JSON . stringify ( suggestion ) , { tabWidth : 4 , filepath : ".json" } )
483+ : JSON . stringify ( suggestion , undefined , 4 ) + "\n"
484+ ) . split ( / ^ / m) ;
485+ const lines = contents . split ( / ^ / m) ;
486+ // When suggestionLines is empty, that suggests removing all
487+ // of the different lines
488+ let startLine = 1 ;
489+ while ( suggestionLines [ 0 ] ?. trim ( ) === lines [ startLine - 1 ] ?. trim ( ) ) {
490+ suggestionLines . shift ( ) ;
491+ startLine ++ ;
492+ }
493+ let endLine = lines . length ;
494+ while ( suggestionLines [ suggestionLines . length - 1 ] ?. trim ( ) === lines [ endLine - 1 ] ?. trim ( ) ) {
495+ suggestionLines . pop ( ) ;
496+ endLine -- ;
497+ }
498+ return {
499+ startLine,
500+ endLine,
501+ body : suggestionLines . join ( "" ) ,
502+ } ;
503+ }
481504 } ;
482505}
483506
@@ -488,7 +511,7 @@ function latestComment(comments: PR_repository_pullRequest_comments_nodes[]) {
488511
489512function getMergeOfferDate ( comments : PR_repository_pullRequest_comments_nodes [ ] , abbrOid : string ) {
490513 const offer = latestComment ( comments . filter ( c =>
491- sameUser ( "typescript-bot" , c . author ?. login || "-" )
514+ ! authorNotBot ( c )
492515 && comment . parse ( c . body ) ?. tag === "merge-offer"
493516 && c . body . includes ( `(at ${ abbrOid } ) ) ) ;
494517 return offer && new Date ( offer . createdAt ) ;
0 commit comments