New Ticket     Wiki     Browse Source     Timeline     Roadmap     Ticket Reports     Search

Ticket #14799 (closed defect: fixed)

Opened 4 years ago

Last modified 3 years ago

reduce 'port lint' pedantry

Reported by: ricci@… Owned by: jmpp@…
Priority: Normal Milestone: MacPorts 1.7.0
Component: base Version: 1.7.0
Keywords: Cc: ryandesign@…, blb@…
Port:

Description

Attached patch reduces 'port lint' pedantry by silencing the following:

whitespace checks (except for newline at EOF). They're invisible to port and to humans.

patchfile naming needs only have 'patch-', no other requirements.

Attachments

patch-src_port1.0_portlint.tcl Download (1.8 KB) - added by ricci@… 4 years ago.
reduce 'port lint' pedantry
portlint.tcl.patchfile-name.diff Download (0.8 KB) - added by ryandesign@… 4 years ago.
be less pedantic about patchfile names
portlint.tcl.blank-lines.diff Download (2.1 KB) - added by ryandesign@… 4 years ago.
don't warn about missing blank lines
portlint.tcl.trailing-whitespace.diff Download (0.6 KB) - added by ryandesign@… 4 years ago.
only warn about trailing whitespace after a backslash

Change History

Changed 4 years ago by ricci@…

reduce 'port lint' pedantry

  Changed 4 years ago by raimue@…

We should discuss the patch naming policy on the list before making changes. I don't know when or how it was decided to be 'patch-*.diff', but why change it to 'patch-*' now and not '*.diff'?

I think this was introduced to make it easier for committers to use patches with their favorite editor. Therefore '*.diff' would match this requirement more than 'patch-*'.

Also, I think this might have reference at other places (the guide?), where it would have to be changed, too.

  Changed 4 years ago by ryandesign@…

  • cc ryandesign@… added

This is not to be implemented until  discussion on the mailing list concludes and a decision has been reached.

Changed 4 years ago by ryandesign@…

be less pedantic about patchfile names

Changed 4 years ago by ryandesign@…

don't warn about missing blank lines

Changed 4 years ago by ryandesign@…

only warn about trailing whitespace after a backslash

follow-up: ↓ 4   Changed 4 years ago by wsiegrist@…

The server's portlint.tcl has been updated to the latest. This fixes some bugs from v1.6, however, it completely breaks patchfile name warnings as a "file exists" check was added to fix warning about upstream patches. Auto linting does not have access to the files/ directory, so it thinks all patches are remote since they dont exist. Once a final decision/patch is decided on for portlint, the patchfile exists issue should be included in that.

in reply to: ↑ 3 ; follow-up: ↓ 5   Changed 4 years ago by ryandesign@…

Replying to wsiegrist@apple.com:

it completely breaks patchfile name warnings as a "file exists" check was added to fix warning about upstream patches. Auto linting does not have access to the files/ directory, so it thinks all patches are remote since they dont exist.

Didn't you cause this issue by making the server's port lint checkout non-recursive in r35474?

in reply to: ↑ 4   Changed 4 years ago by wsiegrist@…

  • owner changed from jberry@… to jmpp@…

Replying to ryandesign@macports.org:

Didn't you cause this issue by making the server's port lint checkout non-recursive in r35474?

The lint process was always non-recursive to minimize the work done by the server. If "file exists" is the only way to enforce the naming policy then I'll fix it, but like I said in my last comment, I am holding off on any further changes until the policy is decided upon. In effect, this disables patch file name errors completely until a decision is made.

I'm going to assign to jmpp for a portmgr decision on this.

  Changed 4 years ago by jmpp@…

  • component changed from ports to base

As I said in my latest post to the dev list, we should:

"not advise on patchfile naming in any way (either if that's patch-*.diff, patch-*, *.diff or *.patch) at the lint level. From the very beginning this was only a suggestion that most of us agreed on, so we can still keep it in the guide and elsewhere as such, a suggestion; but it was just that, a suggestion, so I really don't think we have any business annoying a maintainer who, after reading the guide and what-not, has still chosen to name his patches in whatever other way. So my take on this particular issue is that we should remove all patch naming checks from lint, and as a direct result any [file exists foo] checks, which would instantly resolve Bill's comments here."

So lets wait a little while in case I have to deal with any flames on the list, but otherwise I'd consider the non-recursive nature of the checkouts and no resulting access to patchfiles a non-issue.

-jmpp

  Changed 4 years ago by jmpp@…

  • milestone changed from MacPorts base bugs to MacPorts 1.6.1

  Changed 4 years ago by afb@…

It would be great if someone could add a whitespace option to the "lint" command, so that one can actually use the tool to check the Portfile even if the autonag email doesn't complain anymore...

  Changed 4 years ago by afb@…

like in  portlint(1) options:

-t
    Nit pick about use of spaces.

  Changed 3 years ago by blb@…

  • cc blb@… added

How much of this should go into 1.7.0? Everything but afb's nit-picky-spacey suggestion appears to have patches (though they may not apply cleanly now).

  Changed 3 years ago by ryandesign@…

As of r41170, lint no longer nitpicks whitespace or patchfile names. I think this is sufficient for 1.7.0.

I defined a new variable "nitpick" set to false. There is no exposed mechanism to set it to true. The last step would be to provide a way to set nitpick to true via some command-line switch. Do you know how to do that? I'm not sure I do.

  Changed 3 years ago by blb@…

  • status changed from new to closed
  • resolution set to fixed

nitpick option added to 'port lint' in r41511.

$ port lint cidr
--->  Verifying Portfile for cidr
--->  0 errors and 0 warnings found.
$ port lint --nitpick cidr
--->  Verifying Portfile for cidr
Warning: Line 4 should be a newline (after PortSystem)
--->  0 errors and 1 warnings found.

  Changed 3 years ago by blb@…

And added to port.1 man page in r41512.

Note: See TracTickets for help on using tickets.