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

Use OpenQA::Test::PatchDeparse from os-autoinst-common #4613

Merged
merged 3 commits into from Apr 26, 2022

Conversation

perlpunk
Copy link
Contributor

No description provided.

@perlpunk perlpunk changed the title Use OpenQA::Test::PatchDeparse from s-autoinst-common Use OpenQA::Test::PatchDeparse from os-autoinst-common Apr 20, 2022
@codecov
Copy link

codecov bot commented Apr 20, 2022

Codecov Report

Merging #4613 (35f7f22) into master (3ba6f96) will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4613      +/-   ##
==========================================
+ Coverage   97.99%   98.04%   +0.04%     
==========================================
  Files         375      374       -1     
  Lines       34433    34414      -19     
==========================================
- Hits        33743    33740       -3     
+ Misses        690      674      -16     
Impacted Files Coverage Δ
...os-autoinst-common/lib/OpenQA/Test/PatchDeparse.pm

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ba6f96...35f7f22. Read the comment docs.

@perlpunk perlpunk force-pushed the patch-deparse branch 2 times, most recently from 183475b to 6191001 Compare April 21, 2022 10:03
@perlpunk
Copy link
Contributor Author

I restricted tidy to t/, lib/ and script/

.tidyallrc Outdated
@@ -2,5 +2,5 @@
; Notice that this does include also non-Perl files in script/ so it is
; suggested to use tools/tidy to cover all appropriate files. "tidyall -a"
; will try to cover too much and fail on the non-Perl files
select = **/*.{pl,pm,t} script/*
select = {lib,t}/**/*.{pl,pm,t} script/*
Copy link
Member

Choose a reason for hiding this comment

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

This would exclude tools/ at least. I suggest to use

Suggested change
select = {lib,t}/**/*.{pl,pm,t} script/*
ignore = external/

see https://metacpan.org/pod/tidyall

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently ./tools/tidy -l does not list anything in tools, but ok.

But I'm a fan of explicitly including things. I hate that I have to use ignore in os-autoinst. So I will include tools explicitly.

Copy link
Member

Choose a reason for hiding this comment

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

Well, I prefer to include everything and only exclude so that any new files or directories are not silently ignored by mistake. Having too many files included produces quite explicit errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So one against one. Any other opinions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How many new directories in the root directory containing perl files are we expecting in the future?

Copy link
Member

Choose a reason for hiding this comment

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

That's the point, I don't know. To be safe I suggest to include all and specify the exceptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are some standard directories where perl files live in a project, and then maybe some custom ones. This config file can serve as an easy way to document in which directories we expect perl files that we want to tidy.
I don't get the argument of "being safe", especially when talking about perltidy.

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 convince someone else of the team to vote for the "ignore" option and I'll do it =)

@perlpunk
Copy link
Contributor Author

subrepo:
  subdir:   "external/os-autoinst-common"
  merged:   "120b41ada"
upstream:
  origin:   "git@github.com:os-autoinst/os-autoinst-common.git"
  branch:   "master"
  commit:   "3de570d96"
git-subrepo:
  version:  "0.4.3"
  origin:   "???"
  commit:   "???"
We don'y want to tidy the external/ directory, and also not
dbicdh/ I guesss.
@perlpunk
Copy link
Contributor Author

I will merge this now because the webui-docker-compose failed because of a zypper timeout (ran longer than 1 hour!)
Don't want this to get an endless PR

@perlpunk perlpunk merged commit fb9ca22 into os-autoinst:master Apr 26, 2022
@perlpunk perlpunk deleted the patch-deparse branch April 27, 2022 10:28
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.

None yet

3 participants