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

[feature] generate the report in the desired path #46

Merged
merged 8 commits into from
May 3, 2023

Conversation

KostiantynPopovych
Copy link
Contributor

@KostiantynPopovych KostiantynPopovych commented Apr 13, 2023

This PR adds the ability to generate a report file in the desired path using CLI and API.

@KostiantynPopovych KostiantynPopovych added the enhancement New feature or request label Apr 13, 2023
@KostiantynPopovych KostiantynPopovych self-assigned this Apr 13, 2023
@KostiantynPopovych KostiantynPopovych marked this pull request as ready for review April 14, 2023 21:51
@kindoflew
Copy link
Contributor

@KostiantynPopovych the new feature looks good to me! i especially like the inclusion of a dedicated logging library!

but i have to say i'm against including CLI functionality in the core depngn function. i believe core and cli should remain separate modules, with cli importing depngn and not the other way around. (i know i'm not an actual maintainer of this package anymore, but i feel strongly about this one, so i figured i'd voice my opinion on it).

Summed up arguments against it:

  • it violates the "Single Responsibility" principle in SOLID -- depngn should only output compatibility data and cli should handle the reports (and, by extension, validation).
  • it forces anyone who installs depngn from npm to include ourcli modules, even if they never intend to use them.
  • (subjective) for some reason, i don't like the idea of a function that can return a value or execute a side effect and return undefined. i feel like that's an unexpected/inconsistent public interface. especially because the return changes based on unrelated arguments passed to it (the absence of reporter and reporterPath and not, for example, an argument like returnData: true or something).
  • i think it makes navigating the codebase less straightforward. the way it currently is, you can expect functions used in one module to come from that module (with the exception of depngn being called in the cli function). in this implementation, you have functions from cli being imported into core, which could make it difficult for potential contributors to follow the execution flow. i know it's only a few functions now, but i think strictly separating them is a better convention for future maintenance.

besides all that, it was a conscious decision to keep them separate when i initially wrote this so users could import depngn as an npm package to generate the data themselves and then choose what to do with that output.

jest.config.ts Outdated
@@ -17,7 +17,7 @@ export default {
clearMocks: true,

// Indicates whether the coverage information should be collected while executing the test
collectCoverage: true,
collectCoverage: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

why not collect coverage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just forgot to rollback it

@KostiantynPopovych
Copy link
Contributor Author

Hey @kindoflew! Thank you for reviewing! I wrote this after a break, so the initial approach is not perfect, and your points are fair. I rebuilt it, created a standalone entity - report and now I hope it fixed all previous concerns. I didn't update the related docs so that you can review this again, and after that, I will update README.

@kindoflew
Copy link
Contributor

kindoflew commented Apr 21, 2023

@KostiantynPopovych -- oohh, i didn't even think of having report be a separate module the user can import! neat!

looking over everything, i wonder if including the file name as part of the reportOutputPath is more work for us and the user than we need?

for example -- in istanbul you can choose the report-dir and that only refers to the directory -- the file type is chosen by reporter and i don't think you can customize the file name. i wonder if that is a less complex option -- both for us maintaining and devs using?

the main reason i think this is because, as it is, a user would have to remember and know that if they specify a filename it has to have the matching extension or it throws an error. not the biggest deal in the world, but maybe a little bit of a clunky user experience?

but reportDir is unambiguous -- there's no way to screw it up as long as it's a valid name for a directory. we could also have a reportFileName option that doesn't need the extension -- that could be inferred from reporter. what do you think?

@KostiantynPopovych
Copy link
Contributor Author

@kindoflew, hmmm, I thought having an additional option for file name would be too much, but it will definitely be less complicated for us to maintain and probably for users to get used it. So, yeah, I will split the logic into two separate options (the idea of this PR under the hood is to be able to create a report with some specific file name)

@KostiantynPopovych
Copy link
Contributor Author

@kindoflew, updated, two options are in place)

@KostiantynPopovych KostiantynPopovych merged commit db5ee66 into main May 3, 2023
@KostiantynPopovych KostiantynPopovych deleted the feature/report-from-api branch May 3, 2023 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants