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.

Sign up for GitHub

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

Jump to bottom

Ignore test files in linters #240

Merged
merged 1 commit into from
Jan 9, 2024
Merged

Ignore test files in linters #240

merged 1 commit into from
Jan 9, 2024

Conversation

tuantrantg
Copy link
Contributor

No description provided.

@tuantrantg tuantrantg marked this pull request as draft December 11, 2023 03:15
@tuantrantg tuantrantg marked this pull request as ready for review December 11, 2023 03:32
@tuantrantg tuantrantg mentioned this pull request Dec 11, 2023
Merged
@tuantrantg tuantrantg force-pushed the ignore-tests branch 2 times, most recently from 8748ce1 to d3074b2 Compare December 11, 2023 03:47
@tuantrantg tuantrantg changed the title Ignore tests directories in linters Ignore test directories/test files in linters Dec 11, 2023
@tuantrantg tuantrantg changed the title Ignore test directories/test files in linters Ignore test files in linters Dec 11, 2023
@tuantrantg tuantrantg force-pushed the ignore-tests branch 2 times, most recently from d801436 to b96cf16 Compare December 11, 2023 04:08
simahawk
simahawk reviewed Dec 12, 2023
View reviewed changes
src/.pre-commit-config.yaml.jinja Outdated
@@ -85,6 +85,8 @@ exclude: |
readme/.*\.(rst|md)$|
# Ignore build and dist directories in addons
/build/|/dist/|
# Ignore test files in addons
/tests/.*\.(wamas|txt|xml)$|
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/tests/.*\.(wamas|txt|xml)$|
/tests/fixtures/*|

I would ignore everything in a given dir.
If by chance someone wants to have formatted xmls there the it will be their choices.
Also, I would not tie this to any specific extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @simahawk the PR has been updated as your request

yajo
yajo requested changes Dec 12, 2023
View reviewed changes
Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Could you please explain the motivation behind this PR?

@tuantrantg
Copy link
Contributor Author

Could you please explain the motivation behind this PR?

hi @yajo thanks for asking. I'm doing some modules that need XML / text files to run unitest and pre-commit beautifies XML / text files and I want to prevent to do this

@simahawk
Copy link
Contributor

Could you please explain the motivation behind this PR?

Example: you get an example file (xml maybe) from an official doc/example and you want to respect the original. There's no reason to re-format it.
And it's optional in any case.

yajo
yajo reviewed Dec 14, 2023
View reviewed changes
src/.pre-commit-config.yaml.jinja Outdated
@@ -85,6 +85,8 @@ exclude: |
readme/.*\.(rst|md)$|
# Ignore build and dist directories in addons
/build/|/dist/|
# Ignore test files in addons
/tests/fixtures/*|
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanations.

I think "fixtures" is very confusing here, as it has no special meaning, and otherwise it reminds to pytest fixtures, which have no relationship with what you want.

What about this?

Suggested change
/tests/fixtures/*|
/tests/vendor/*|

"Vendor" is a common word for external code or files that are included in an alien repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine for me. @tuantrantg can you update?

Copy link
Member

Choose a reason for hiding this comment

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

I would recommend /tests/resources as a more generic name for files which the tests depend on. This directory needn't necessarily contain 3rd-party stuff. Failing that, /tests/vendor is also fine.

Copy link
Member

Choose a reason for hiding this comment

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

Or even samples better than vendor one IMO.

@nilshamerlinck nilshamerlinck mentioned this pull request Dec 27, 2023
Merged
@QuocDuong1306 QuocDuong1306 mentioned this pull request Jan 3, 2024
Closed
@tuantrantg
Copy link
Contributor Author

hello @pedrobaeza @carmenbianca @simahawk @yajo I finally updated this to /tests/samples

@simahawk
Copy link
Contributor

simahawk commented Jan 9, 2024

@yajo fine for you?

@yajo yajo merged commit 5311787 into OCA:master Jan 9, 2024
8 checks passed
@legalsylvain legalsylvain mentioned this pull request Apr 2, 2024
Merged
@simahawk simahawk mentioned this pull request Apr 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carmenbianca carmenbianca carmenbianca left review comments

@simahawk simahawk simahawk approved these changes

@yajo yajo yajo approved these changes

@pedrobaeza pedrobaeza pedrobaeza approved these changes

Assignees
No one assigned
Labels
approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tuantrantg @simahawk @yajo @pedrobaeza @carmenbianca @OCA-git-bot