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

lint W47: clarify empty description message #5069

Merged
merged 3 commits into from
Feb 14, 2023
Merged

Conversation

rjbou
Copy link
Collaborator

@rjbou rjbou commented Feb 25, 2022

Specify that descr section is empty instead of description remove descr note in order to deprecate it
fix #4989

-warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
+warning 47: Synopsis  should start with a capital and not end with a dot

@rjbou rjbou added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Feb 25, 2022
@rjbou rjbou added this to the 2.2.0~alpha milestone Feb 25, 2022
@rjbou rjbou added this to PR in progress in Opam 2.2.0 via automation Feb 25, 2022
@rjbou rjbou moved this from PR in progress to PR to review in Opam 2.2.0 Feb 28, 2022
@rjbou rjbou removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Feb 28, 2022
@@ -524,8 +524,8 @@ let t_lint ?check_extra_files ?(check_upstream=false) ?(all=false) t =
(t.install <> [] || t.remove <> [] || t.url <> None ||
t.extra_sources <> []));
cond 47 `Warning
"Synopsis (or description first line) should start with a capital and \
not end with a dot"
"Synopsis (or 'descr' section first line) should start with a capital \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Synopsis (or 'descr' section first line) should start with a capital \
"Synopsis should start with a capital \

Should we maybe entierly omit this, given this is opam 1.2 stuff?

Copy link
Member

Choose a reason for hiding this comment

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

It would make the warning message clearer and more to the point

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does all packages have a synopsis ? Lint 57 just checks if synopsis and descr are not both empty

Copy link
Member

Choose a reason for hiding this comment

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

Good question. I was under the impression that synopsis was "mandatory" (would raise a warning if missing), but now that #5070 has been merged it’s not the case anymore.

I think it should be mandatory given currently if it is not present, opam list will not show the "first line of the description field" as this W47 might indicate:

opam list hll.3.16
# Packages matching: name-match(hll) & version-match(3.16) & (installed | available)
# Package # Installed # Synopsis
hll.3.16  --

(hll.3.16 currently does not have synopsis but a one line description)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#5070 doesn't change the lint behaviour, it only fixes double linting trigger (empty synopsis | description that trigger no capital one).

Cited list behaviour for me is a list issue as we didn't defined that synopsis is mandatory & non empty (which is the case of hll)

@rjbou
Copy link
Collaborator Author

rjbou commented Mar 16, 2022

from dev meeting: We need to check synopsis & description usage. synopsis must not be empty, and description is optional. it's descr that is optional -> add a deprectaed message when encountered (in order to remove it)

@rjbou
Copy link
Collaborator Author

rjbou commented Mar 16, 2022

proposal: in this PR, we remove the descr reference, and in another, we deprecate and clean-up OpamFile.Descr usage

master_changes.md Outdated Show resolved Hide resolved
@dra27
Copy link
Member

dra27 commented Jan 23, 2023

This just needs:

  • Changes entry updating, as it got out of sync with the PR
  • An issue to be opened to track adding a new lint warning for missing synopsis

@dra27 dra27 moved this from PR to review to PR to review for 2.2.0~alpha in Opam 2.2.0 Jan 23, 2023
@kit-ty-kate
Copy link
Member

Thanks!

@kit-ty-kate kit-ty-kate merged commit 24e5326 into ocaml:master Feb 14, 2023
Opam 2.2.0 automation moved this from PR to review for 2.2.0~alpha to Done Feb 14, 2023
@kit-ty-kate
Copy link
Member

The followup PR to restore the missing lint is at #5442

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Opam 2.2.0
  
Done
Development

Successfully merging this pull request may close these issues.

Bug: incorrect warning 47?
3 participants