ScottLogic / openapi-forge

⚒️🔥 Effortlessly create OpenAPI clients, in a range of languages, from the fiery furnace of our forge
8 stars 7 forks source link

Feature: Recursive directory copying for files in template/ #149

Closed ms14981 closed 1 year ago

ms14981 commented 1 year ago

What:

Why:

Caveats:

ColinEberhardt commented 1 year ago

Nice work, the refactor and addition of unit tests are great 👍

Before merging, can you consider a possible change to the tests? I find the code that mocks the folder structure a little hard to read:

fs.readdirSync.mockImplementation((filePath, { withFileTypes } = {}) => {
      if (filePath.includes("helpers") || filePath.includes("partials")) {
        return [];
      } else if (withFileTypes) {
        return [
          {
            name: `${fileName}.${fileExtension}`,
            isDirectory: () => false,
          },
        ];
      } else {
        return [`${fileName}.${fileExtension}`];
      }
    });

How easy do you think it would be to create something more 'visual'. For example:

const readdirMock = mockReadDir({
   helpers: [],
   partials: [],
   [${fileName}.${fileExtension}]: ""
});

I know that's not quite right - but basically represent the mocked folder structure as an object literal.

Colin E.

ColinEberhardt commented 1 year ago

One other small point:

we need to output our transformed files in Maven's directory structure (src/main/java//)

I presume you've seen the -o CLI option that specifies the output folder? regardless, allowing a nested structure within the template folder is a very useful feature.

ms14981 commented 1 year ago

One other small point:

we need to output our transformed files in Maven's directory structure (src/main/java//)

I presume you've seen the -o CLI option that specifies the output folder? regardless, allowing a nested structure within the template folder is a very useful feature.

Yes, the -o option will definitely cover most cases. However, it won't cover the case where we might want to generate a pom.xml file in the top level directory, and the rest of the files elsewhere. Currently, the forge will crash if there are any folders nested in the template folder.

ms14981 commented 1 year ago

Nice work, the refactor and addition of unit tests are great 👍

Before merging, can you consider a possible change to the tests? I find the code that mocks the folder structure a little hard to read:

fs.readdirSync.mockImplementation((filePath, { withFileTypes } = {}) => {
      if (filePath.includes("helpers") || filePath.includes("partials")) {
        return [];
      } else if (withFileTypes) {
        return [
          {
            name: `${fileName}.${fileExtension}`,
            isDirectory: () => false,
          },
        ];
      } else {
        return [`${fileName}.${fileExtension}`];
      }
    });

How easy do you think it would be to create something more 'visual'. For example:

const readdirMock = mockReadDir({
   helpers: [],
   partials: [],
   [${fileName}.${fileExtension}]: ""
});

I know that's not quite right - but basically represent the mocked folder structure as an object literal.

Colin E.

Good shout, I've made a few changes which hopefully will make those mocks easier to read and work with :)

gkocak-scottlogic commented 1 year ago

All seems fine to me! I tried the the branch locally as well 👍

github-actions[bot] commented 1 year ago

:tada: This PR is included in version 1.3.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: