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

feat: add ability to define number of generated extensions in system.filePath #395

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions src/system.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,12 +174,32 @@ export class System {
/**
* Returns a file path.
*
* @param ext Defines number of generated extensions. Defaults to `1`.
*
* @example
* faker.system.filePath() // '/usr/local/src/money.dotx'
* faker.system.filePath(0) // '/usr/local/src/money'
* faker.system.filePath(2) // '/usr/local/src/money.dotx.zip'
*/
// TODO @prisis 2022-01-25: add a parameter to have the possibility to have one or two ext on file.
filePath(): string {
return `${this.directoryPath()}/${this.fileName()}`;
filePath(ext = 1): string {
Copy link
Member

Choose a reason for hiding this comment

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

Should we make it also possible to set the nested dir depth?
So we would need to forward an arg to directoryPath
So IMO we should use an option here {extCount = 1, dirCount: 1-3?}

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, why not! :) I can do that once #300 is merged, ok?

Copy link
Member

Choose a reason for hiding this comment

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

sure

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, for now stick to a fixed number for each of them.
In a later feature we can introduce number | { min, max } (that might be handy for multiple locations)
Words, Lorem, ...

Copy link
Member Author

Choose a reason for hiding this comment

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

directoryPath does not take any params. I would propose to extend it in a separate PR not to mix the concerns.

Copy link
Member

Choose a reason for hiding this comment

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

directory path in separate PR: OK

const path = `${this.directoryPath()}/${this.fileName()}`;

switch (true) {
case ext === 0:
return path.slice(0, path.lastIndexOf('.'));

case ext === 1:
return path;

default: {
const extensions = new Array(ext - 1)
.fill('')
.map(() => this.fileExt())
.join('.');

return `${path}.${extensions}`;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move this logic into fileName()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, let me do that...

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, Mayne I should do that in a separate PR not to mix concerts @ST-DDT ? It would be also better to spot on the changelog?

Copy link
Member

@ST-DDT ST-DDT Apr 6, 2022

Choose a reason for hiding this comment

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

This PR is about changing the number of generated extensions,
I think that its acceptable to do so in two/three related methods within a single PR.

feat: add ability to define the number of generated file extensions

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we just need to adjust the PR title a little bit 🤷
But I also think it's okay to provide this in one PR

}

/**
Expand Down
27 changes: 23 additions & 4 deletions test/system.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,13 @@ describe('system', () => {
'htm',
'html',
'jpeg',
'm1v',
'm2a',
'm2v',
'mp2',
'mp3',
'm2v',
'mp2a',
'mp4',
'mp4v',
'mpeg',
Expand Down Expand Up @@ -234,16 +237,32 @@ describe('system', () => {
describe('filePath()', () => {
it('should return unix fs file full path', () => {
const filePath = faker.system.filePath();
const parts = filePath.split('/');
const file = filePath.split('/').pop();

expect(
filePath.startsWith('/'),
'generated filePath should start with /'
).toBeTruthy();

expect(
parts[parts.length - 1],
'generated filePath should have a file extension'
).toMatch(/^\w+\.\w+$/);
file.includes('.'),
'generated filePath should have extension'
).toBeTruthy();
});

it('should not have extension when ext=0', () => {
const filePath = faker.system.filePath(0);
const file = filePath.split('/').pop();

expect(file.includes('.')).toBeFalsy();
});

it('should have multiple extension when ext > 1', () => {
const ext = 3;
const filePath = faker.system.filePath(ext);
const file = filePath.split('/').pop();

expect(file.split('.').length).toBe(ext + 1);
});
});

Expand Down