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

Add backend test framework template #475

Merged

Conversation

mishayouknowme
Copy link
Contributor

No description provided.

Copy link

linear bot commented Dec 14, 2023

PLA-256

@mishayouknowme mishayouknowme self-assigned this Dec 18, 2023
src/backend-test/index.ts Outdated Show resolved Hide resolved
src/backend-test/index.ts Outdated Show resolved Hide resolved
src/backend-test/index.ts Outdated Show resolved Hide resolved
src/backend-test/assets/codegen.ts Outdated Show resolved Hide resolved
src/backend-test/index.ts Outdated Show resolved Hide resolved
src/backend-test/__tests__/index.ts Show resolved Hide resolved
src/backend-test/index.ts Outdated Show resolved Hide resolved
src/backend-test/assets/tests/auth.spec.ts.sample Outdated Show resolved Hide resolved
src/backend-test/assets/common/index.ts.sample Outdated Show resolved Hide resolved
src/backend-test/assets/common/client.ts.sample Outdated Show resolved Hide resolved
src/backend-test/assets/api/auth/response.ts.sample Outdated Show resolved Hide resolved
src/backend-test/assets/api/auth/request.ts.sample Outdated Show resolved Hide resolved
@mishayouknowme mishayouknowme force-pushed the michaelpopov1/pla-256-add-backend-test-framework-template branch 2 times, most recently from e058aae to 9c5754a Compare January 2, 2024 12:48
@quesabe
Copy link
Collaborator

quesabe commented Jan 5, 2024

Please, rebase into the main branch as there are plenty of changes accumulated.

src/backend-test/index.ts Show resolved Hide resolved

// ANCHOR ESLint and prettier setup

const lintPaths = options.lintPaths ?? ['.projenrc.ts', '.']
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why wouldn't you include explicitly the patterns you want to check and instead includes everything? I'm not sure we should use the format command on all the projen-generated files. For now I did not find any conflicts when the command is run. Though there is no guarantee for the future, as we can change/add our rules which could contradict what projen renders.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, it's not completely clear to me what you mean by including explicitly the patterns. Currently, I am not passing the projenrc file to the addLinters and just using [.] it will be enough?

Copy link
Collaborator

Choose a reason for hiding this comment

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

By include explicitly the patterns you want I mean getting rid of . and manually list the files and folders you want to format/check. Thus you would have the same list defined for both eslint and prettier. On the other hand with . you have all files processed (even the readme) and if anything needs be excluded one must handle separate ignore lists for prettier and eslint.

However I don't insist. If that's what you expect from the project, then I'm ok with it. Maybe @bdoof has any comment on it.

})

// ANCHOR ESLint and prettier setup

Copy link
Collaborator

Choose a reason for hiding this comment

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

An excessive blank line


// ANCHOR ESLint and prettier setup

const lintPaths = options.lintPaths ?? ['.projenrc.ts', '.']
Copy link
Collaborator

Choose a reason for hiding this comment

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

By include explicitly the patterns you want I mean getting rid of . and manually list the files and folders you want to format/check. Thus you would have the same list defined for both eslint and prettier. On the other hand with . you have all files processed (even the readme) and if anything needs be excluded one must handle separate ignore lists for prettier and eslint.

However I don't insist. If that's what you expect from the project, then I'm ok with it. Maybe @bdoof has any comment on it.

@gvidon
Copy link
Contributor

gvidon commented Jan 20, 2024

@quesabe @mishayouknowme what's the status of this PR? How close are we to merging it?

@quesabe
Copy link
Collaborator

quesabe commented Jan 24, 2024

@quesabe @mishayouknowme what's the status of this PR? How close are we to merging it?

From my side I marked two minor issues (which do not stop us from merging):

  • an unnecessary empty line
  • and a controversial listing of the . pattern (which would possibly result in prettier formatting generated files); as much as I don't like it I'm ready to accept it if the template users are ok.

I didn't check whether the changes requested by @bdoof are done - I expected him to verify and resolve the conversations he opened.

Overall we seem to be very close to the end line. Just need to coordinate the remaining concerns.

And final point: I'd prefer all the commits to be squashed before merging. Otherwise we'll get a lot of irrelevant historical info in the main branch.

@bdoof
Copy link
Contributor

bdoof commented Jan 29, 2024

I think . is OK for prettier/eslint configs.
We use .prettierignore & .eslintignore to define what we need to skip.
Usually, we check all files with a few exceptions (GraphQL codegen, for example).

@bdoof
Copy link
Contributor

bdoof commented Jan 31, 2024

@quesabe @mishayouknowme are we good to merge it?

@quesabe quesabe merged commit 4c1d48d into main Feb 1, 2024
6 checks passed
@quesabe quesabe deleted the michaelpopov1/pla-256-add-backend-test-framework-template branch February 1, 2024 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants