Skip to content

Commit

Permalink
Refactor: don't sanitize path separators in game & ROM names (#1019)
Browse files Browse the repository at this point in the history
  • Loading branch information
@emmercm
emmercm authored Mar 20, 2024
1 parent facb20e commit 2063004
Show file tree
Hide file tree
Showing 9 changed files with 46 additions and 142 deletions.
26 changes: 10 additions & 16 deletions src/modules/candidatePatchGenerator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ export default class CandidatePatchGenerator extends Module {
entryPath: unpatchedReleaseCandidate.getRomsWithFiles().length === 1
? extractedFileName
: outputFile.getEntryPath(),
size: patch.getSizeAfter() ?? 0,
size: patch.getSizeAfter(),
crc32: patch.getCrcAfter(),
fileHeader: outputFile.getFileHeader(),
patch: outputFile.getPatch(),
Expand All @@ -159,21 +159,18 @@ export default class CandidatePatchGenerator extends Module {
const dirName = path.dirname(outputFile.getFilePath());
outputFile = await File.fileOf({
filePath: path.join(dirName, extractedFileName),
size: patch.getSizeAfter() ?? 0,
size: patch.getSizeAfter(),
crc32: patch.getCrcAfter(),
fileHeader: outputFile.getFileHeader(),
patch: outputFile.getPatch(),
}, ChecksumBitmask.NONE); // don't calculate anything, the file doesn't exist
}

// Build a new ROM from the output file's info
let romName = path.basename(outputFile.getExtractedFilePath());
if (dat.getRomNamesContainDirectories()) {
romName = path.join(
path.dirname(rom.getName().replace(/[\\/]/g, path.sep)),
romName,
);
}
const romName = path.join(
path.dirname(rom.getName().replace(/[\\/]/g, path.sep)),
path.basename(outputFile.getExtractedFilePath()),
);
rom = new ROM({
name: romName,
size: outputFile.getSize(),
Expand All @@ -187,13 +184,10 @@ export default class CandidatePatchGenerator extends Module {
}));

// Build a new Game from the ROM's info
let gameName = patchedRomName;
if (dat.getRomNamesContainDirectories()) {
gameName = path.join(
path.dirname(unpatchedReleaseCandidate.getGame().getName().replace(/[\\/]/g, path.sep)),
gameName,
);
}
const gameName = path.join(
path.dirname(unpatchedReleaseCandidate.getGame().getName().replace(/[\\/]/g, path.sep)),
patchedRomName,
);
const patchedGame = unpatchedReleaseCandidate.getGame().withProps({
name: gameName,
});
Expand Down
1 change: 0 additions & 1 deletion src/modules/datCombiner.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ export default class DATCombiner extends Module {
`Combined DAT generated by ${Constants.COMMAND_NAME} v${Constants.COMMAND_VERSION}`,
...dats.map((dat) => dat.getName()),
].join('\n'),
romNamesContainDirectories: dats.some((dat) => dat.getRomNamesContainDirectories()),
});
}
}
6 changes: 1 addition & 5 deletions src/modules/datMergerSplitter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import ProgressBar, { ProgressBarSymbol } from '../console/progressBar.js';
import ArrayPoly from '../polyfill/arrayPoly.js';
import DAT from '../types/dats/dat.js';
import Game from '../types/dats/game.js';
import Header from '../types/dats/logiqx/header.js';
import LogiqxDAT from '../types/dats/logiqx/logiqxDat.js';
import Machine from '../types/dats/mame/machine.js';
import Parent from '../types/dats/parent.js';
Expand Down Expand Up @@ -50,10 +49,7 @@ export default class DATMergerSplitter extends Module {

const newGames = dat.getParents()
.flatMap((parent) => this.mergeParent(dat, parent, gameNamesToGames));
const newDat = new LogiqxDAT(new Header({
...dat.getHeader(),
romNamesContainDirectories: this.options.getMergeRoms() === MergeMode.MERGED,
}), newGames);
const newDat = new LogiqxDAT(dat.getHeader(), newGames);
this.progressBar.logTrace(`${newDat.getNameShort()}: merged/split to ${newDat.getGames().length.toLocaleString()} game${newDat.getGames().length !== 1 ? 's' : ''}`);

this.progressBar.logTrace(`${newDat.getNameShort()}: done merging & splitting`);
Expand Down
1 change: 0 additions & 1 deletion src/modules/datScanner.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,6 @@ export default class DATScanner extends Scanner {
return new LogiqxDAT(new Header({
name: datName,
description: datName,
romNamesContainDirectories: true,
}), games);
}

Expand Down
10 changes: 0 additions & 10 deletions src/types/dats/dat.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,16 +94,6 @@ export default abstract class DAT {
return this.getHeader().getDescription();
}

@Memoize()
getRomNamesContainDirectories(): boolean {
return this.getHeader().getRomNamesContainDirectories() ?? (
// Assume BIOS DATs know what they're doing with path characters
this.isBiosDat()
// At least 50% of games have at least one ROM with path characters
|| this.getGames().filter((game) => game.getRoms().some((rom) => rom.getName().match(/[\\/]/) !== null)).length > this.getGames().length / 2
);
}

/**
* Get a No-Intro style filename.
*/
Expand Down
12 changes: 0 additions & 12 deletions src/types/dats/logiqx/header.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,6 @@ interface HeaderOptions {
readonly comment?: string;
readonly clrMamePro?: ClrMamePro;
// readonly romCenter?: RomCenter;

/**
* igir-custom properties.
*/
readonly romNamesContainDirectories?: boolean;
}

/**
Expand Down Expand Up @@ -78,8 +73,6 @@ export default class Header implements HeaderOptions {
@Transform(({ value }) => value || undefined)
readonly clrMamePro?: ClrMamePro;

readonly romNamesContainDirectories?: boolean;

constructor(options?: HeaderOptions) {
this.name = options?.name ?? '';
this.description = options?.description;
Expand All @@ -89,7 +82,6 @@ export default class Header implements HeaderOptions {
this.url = options?.url;
this.comment = options?.comment;
this.clrMamePro = options?.clrMamePro;
this.romNamesContainDirectories = options?.romNamesContainDirectories;
}

/**
Expand Down Expand Up @@ -129,10 +121,6 @@ export default class Header implements HeaderOptions {
return this.clrMamePro;
}

getRomNamesContainDirectories(): boolean | undefined {
return this.romNamesContainDirectories;
}

// Computed getters

/**
Expand Down
13 changes: 4 additions & 9 deletions src/types/outputFactory.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@ export default class OutputFactory {
return `${game.getName()}.zip`;
}

const romBasename = this.getRomBasename(options, dat, rom, inputFile);
const romBasename = this.getRomBasename(game, rom, inputFile);

if (
!(inputFile instanceof ArchiveEntry || FileFactory.isArchive(inputFile.getFilePath()))
Expand All @@ -512,7 +512,7 @@ export default class OutputFactory {
rom: ROM,
inputFile: File,
): string {
const romBasename = this.getRomBasename(options, dat, rom, inputFile);
const romBasename = this.getRomBasename(game, rom, inputFile);
if (!options.shouldZipFile(rom.getName())) {
return romBasename;
}
Expand All @@ -527,16 +527,11 @@ export default class OutputFactory {
}

private static getRomBasename(
options: Options,
dat: DAT,
game: Game,
rom: ROM,
inputFile: File,
): string {
let romNameSanitized = rom.getName();
if (!dat.getRomNamesContainDirectories()) {
romNameSanitized = romNameSanitized?.replace(/[\\/]/g, '_');
}

const romNameSanitized = rom.getName().replace(/[\\/]/g, path.sep);
const { base, ...parsedRomPath } = path.parse(romNameSanitized);

// Alter the output extension of the file
Expand Down
62 changes: 31 additions & 31 deletions test/modules/candidatePostProcessor.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,9 @@ it('should do nothing with no options', async () => {
path.join('Output', 'Dazzling', 'Dazzling.cue'),
path.join('Output', 'Dedicated', 'Dedicated (Track 01).bin'),
path.join('Output', 'Dedicated', 'Dedicated.cue'),
path.join('Output', 'disk1_Cheerful.rom'),
path.join('Output', 'disk1_Confident.rom'),
path.join('Output', 'disk1_Cool.rom'),
path.join('Output', 'disk1', 'Cheerful.rom'),
path.join('Output', 'disk1', 'Confident.rom'),
path.join('Output', 'disk1', 'Cool.rom'),
]);
});

Expand All @@ -120,9 +120,9 @@ describe('dirLetterLimit', () => {
path.join('Output', 'D', 'Dazzling', 'Dazzling.cue'),
path.join('Output', 'D', 'Dedicated', 'Dedicated (Track 01).bin'),
path.join('Output', 'D', 'Dedicated', 'Dedicated.cue'),
path.join('Output', 'D', 'disk1_Cheerful.rom'),
path.join('Output', 'D', 'disk1_Confident.rom'),
path.join('Output', 'D', 'disk1_Cool.rom'),
path.join('Output', 'D', 'disk1', 'Cheerful.rom'),
path.join('Output', 'D', 'disk1', 'Confident.rom'),
path.join('Output', 'D', 'disk1', 'Cool.rom'),
]],
[2, [
path.join('Output', 'A1', 'Admirable.rom'),
Expand All @@ -140,9 +140,9 @@ describe('dirLetterLimit', () => {
path.join('Output', 'D2', 'Dazzling', 'Dazzling.cue'),
path.join('Output', 'D2', 'Dedicated', 'Dedicated (Track 01).bin'),
path.join('Output', 'D2', 'Dedicated', 'Dedicated.cue'),
path.join('Output', 'D3', 'disk1_Cheerful.rom'),
path.join('Output', 'D3', 'disk1_Confident.rom'),
path.join('Output', 'D4', 'disk1_Cool.rom'),
path.join('Output', 'D3', 'disk1', 'Cheerful.rom'),
path.join('Output', 'D3', 'disk1', 'Confident.rom'),
path.join('Output', 'D3', 'disk1', 'Cool.rom'),
]],
[3, [
path.join('Output', 'A1', 'Admirable.rom'),
Expand All @@ -160,9 +160,9 @@ describe('dirLetterLimit', () => {
path.join('Output', 'D1', 'Dazzling', 'Dazzling.cue'),
path.join('Output', 'D2', 'Dedicated', 'Dedicated (Track 01).bin'),
path.join('Output', 'D2', 'Dedicated', 'Dedicated.cue'),
path.join('Output', 'D2', 'disk1_Cheerful.rom'),
path.join('Output', 'D2', 'disk1_Confident.rom'),
path.join('Output', 'D3', 'disk1_Cool.rom'),
path.join('Output', 'D2', 'disk1', 'Cheerful.rom'),
path.join('Output', 'D2', 'disk1', 'Confident.rom'),
path.join('Output', 'D2', 'disk1', 'Cool.rom'),
]],
])('it should split the letter dirs based on limit: %s', async (dirLetterLimit, expectedFilePaths) => {
const options = new Options({
Expand Down Expand Up @@ -204,9 +204,9 @@ describe('dirLetterGroup', () => {
path.join('Output', 'A-D', 'Dazzling', 'Dazzling.cue'),
path.join('Output', 'A-D', 'Dedicated', 'Dedicated (Track 01).bin'),
path.join('Output', 'A-D', 'Dedicated', 'Dedicated.cue'),
path.join('Output', 'A-D', 'disk1_Cheerful.rom'),
path.join('Output', 'A-D', 'disk1_Confident.rom'),
path.join('Output', 'A-D', 'disk1_Cool.rom'),
path.join('Output', 'A-D', 'disk1', 'Cheerful.rom'),
path.join('Output', 'A-D', 'disk1', 'Confident.rom'),
path.join('Output', 'A-D', 'disk1', 'Cool.rom'),
]],
[1, 2, [
path.join('Output', 'A-A1', 'Admirable.rom'),
Expand All @@ -224,9 +224,9 @@ describe('dirLetterGroup', () => {
path.join('Output', 'D-D1', 'Dazzling', 'Dazzling.cue'),
path.join('Output', 'D-D2', 'Dedicated', 'Dedicated (Track 01).bin'),
path.join('Output', 'D-D2', 'Dedicated', 'Dedicated.cue'),
path.join('Output', 'D-D2', 'disk1_Cheerful.rom'),
path.join('Output', 'D-D3', 'disk1_Confident.rom'),
path.join('Output', 'D-D3', 'disk1_Cool.rom'),
path.join('Output', 'D-D2', 'disk1', 'Cheerful.rom'),
path.join('Output', 'D-D2', 'disk1', 'Confident.rom'),
path.join('Output', 'D-D2', 'disk1', 'Cool.rom'),
]],
[1, 3, [
path.join('Output', 'A-A', 'Admirable.rom'),
Expand All @@ -240,13 +240,13 @@ describe('dirLetterGroup', () => {
path.join('Output', 'B-D', 'Dainty', 'Dainty.cue'),
path.join('Output', 'B-D', 'Daring', 'Daring (Track 01).bin'),
path.join('Output', 'B-D', 'Daring', 'Daring.cue'),
path.join('Output', 'D-D1', 'Dazzling', 'Dazzling (Track 01).bin'),
path.join('Output', 'D-D1', 'Dazzling', 'Dazzling.cue'),
path.join('Output', 'D-D1', 'Dedicated', 'Dedicated (Track 01).bin'),
path.join('Output', 'D-D1', 'Dedicated', 'Dedicated.cue'),
path.join('Output', 'D-D1', 'disk1_Cheerful.rom'),
path.join('Output', 'D-D2', 'disk1_Confident.rom'),
path.join('Output', 'D-D2', 'disk1_Cool.rom'),
path.join('Output', 'D-D', 'Dazzling', 'Dazzling (Track 01).bin'),
path.join('Output', 'D-D', 'Dazzling', 'Dazzling.cue'),
path.join('Output', 'D-D', 'Dedicated', 'Dedicated (Track 01).bin'),
path.join('Output', 'D-D', 'Dedicated', 'Dedicated.cue'),
path.join('Output', 'D-D', 'disk1', 'Cheerful.rom'),
path.join('Output', 'D-D', 'disk1', 'Confident.rom'),
path.join('Output', 'D-D', 'disk1', 'Cool.rom'),
]],
[2, 3, [
path.join('Output', 'AD-AD', 'Admirable.rom'),
Expand All @@ -264,9 +264,9 @@ describe('dirLetterGroup', () => {
path.join('Output', 'DA-DI', 'Dazzling', 'Dazzling.cue'),
path.join('Output', 'DA-DI', 'Dedicated', 'Dedicated (Track 01).bin'),
path.join('Output', 'DA-DI', 'Dedicated', 'Dedicated.cue'),
path.join('Output', 'DA-DI', 'disk1_Cheerful.rom'),
path.join('Output', 'DI-DI', 'disk1_Confident.rom'),
path.join('Output', 'DI-DI', 'disk1_Cool.rom'),
path.join('Output', 'DA-DI', 'disk1', 'Cheerful.rom'),
path.join('Output', 'DA-DI', 'disk1', 'Confident.rom'),
path.join('Output', 'DA-DI', 'disk1', 'Cool.rom'),
]],
[3, 4, [
path.join('Output', 'ADM-AMA', 'Admirable.rom'),
Expand All @@ -284,9 +284,9 @@ describe('dirLetterGroup', () => {
path.join('Output', 'DAR-DIS', 'Dazzling', 'Dazzling.cue'),
path.join('Output', 'DAR-DIS', 'Dedicated', 'Dedicated (Track 01).bin'),
path.join('Output', 'DAR-DIS', 'Dedicated', 'Dedicated.cue'),
path.join('Output', 'DAR-DIS', 'disk1_Cheerful.rom'),
path.join('Output', 'DIS-DIS', 'disk1_Confident.rom'),
path.join('Output', 'DIS-DIS', 'disk1_Cool.rom'),
path.join('Output', 'DAR-DIS', 'disk1', 'Cheerful.rom'),
path.join('Output', 'DAR-DIS', 'disk1', 'Confident.rom'),
path.join('Output', 'DAR-DIS', 'disk1', 'Cool.rom'),
]],
])('it should group based on count & limit: %s, %s', async (dirLetterCount, dirLetterLimit, expectedFilePaths) => {
const options = new Options({
Expand Down
57 changes: 0 additions & 57 deletions test/outputFactory.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,63 +58,6 @@ test.each([
});
});

describe('ROM names with directories', () => {
const rom = new ROM({ name: 'subdir\\file.rom', size: 0, crc32: '00000000' });
const game = new Game({ name: 'Game', rom: [rom] });

describe('command: zip', () => {
const options = new Options({ commands: ['copy', 'zip'], output: os.devNull });

test.each([
[true, path.join('subdir', 'file.rom')],
[false, 'subdir_file.rom'],
])('should respect romNamesContainDirectories: %s', async (romNamesContainDirectories, expectedEntryPath) => {
const dat = new LogiqxDAT(new Header({ romNamesContainDirectories }), [game]);

const outputPath = OutputFactory.getPath(
options,
dat,
game,
dummyRelease,
rom,
await rom.toFile(),
);
expect(outputPath).toEqual({
root: '',
dir: os.devNull,
base: '',
name: 'Game',
ext: '.zip',
entryPath: expectedEntryPath,
});
});
});

describe.each([
[['copy']],
[['copy', 'extract']],
])('commands: %s', (commands) => {
const options = new Options({ commands, output: os.devNull });

test.each([
[true, path.join(os.devNull, 'subdir', 'file.rom')],
[false, path.join(os.devNull, 'subdir_file.rom')],
])('should respect romNamesContainDirectories: %s', async (romNamesContainDirectories, expectedFormattedPath) => {
const dat = new LogiqxDAT(new Header({ romNamesContainDirectories }), [game]);

const outputPath = OutputFactory.getPath(
options,
dat,
game,
dummyRelease,
rom,
await rom.toFile(),
);
expect(outputPath.format()).toEqual(expectedFormattedPath);
});
});
});

describe('token replacement', () => {
test.each([
['foo/{datName}/bar', path.join('foo', 'DAT _ Name', 'bar', 'Dummy.rom')],
Expand Down

0 comments on commit 2063004

Please sign in to comment.