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

Line edits for Manual doc #5641

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

christinerose
Copy link
Contributor

Please update master_changes.md file with your changes.

As requested in my previous PR, I've separated commits between Capitalisation, Grammar/Punctuation, and Syntax/Rewording. Hopefully it's less tedious for reviewers.

Please ensure my changes are still accurate. I'll have more questions in the PR to establish clarity in several places as well.
@christinerose
Copy link
Contributor Author

Need clarification:

li 87-90

Repository selection is always ordered, with the
definition of a given version of a package being taken from the repository with
the lowest index where it is found.

Would it be correct to say: "Repository selection is always ordered. The
definition of a given package version is taken from the repository with
the lowest index where it is found."

Even this is still a little confusing. What does does "where it is found" mean? In the same directory?

@christinerose
Copy link
Contributor Author

There are several other comments/questions I have about this doc, mostly for clarification, but I'll wait to ask any more until these are addressed (to avoid too much noise and confusion all at once!)

@christinerose
Copy link
Contributor Author

Before I forget my other questions, I'll add them now, so they maintainers can get to these comments/suggestions when they have time.

Do these suggestions this make it more clear?
Comment on lines +106 to 109
- For local switches, which are external to the opam root, when in the directory
where the switch resides or a descendant
- By setting the `OPAMSWITCH=<switch>` environment variable, to set it within a
single shell session. This can be done by running `eval $(opam env --switch
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
- For local switches, which are external to the opam root, when in the directory
where the switch resides or a descendant
- By setting the `OPAMSWITCH=<switch>` environment variable, to set it within a
single shell session. This can be done by running `eval $(opam env --switch
- For local switches, which are external to the opam root, when in the directory
where the switch resides or a descendant
- By setting the `OPAMSWITCH=<switch>` environment variable, to set it within a
single shell session. This can be done by running `eval $(opam env --switch

This could use some clarification.
"For local switches..." there is no action here. It doesn't say how to select it, or if it does, it's not clear.
There is also no action in the second one: "by setting the ...."
Would you mind explaining these further?

Also, should opam root be opam root?

This is generally done through `sudo`, and always after prompting the user
(unless `--yes` was specified). if disabled, the installation command is
This is generally done through `sudo` and always after prompting the user
(unless `--yes` was specified). If disabled, the installation command is
printed to stdout, and opam pauses to let the user proceed.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
printed to stdout, and opam pauses to let the user proceed.
printed to `stdout`, and opam pauses to let the user proceed.

modified during the installation of the package.
Note that this hook is run after the scan for installed files is
done, so any additional installed files won't be recorded and must be taken
Note that this hook is run after the scan for installed files completes, so any additional installed files won't be recorded and must be taken
care of by a `pre-remove-commands` hook. However, modified or deleted installed
files during the `post-install-commands` will be handled correctly by `opam`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
files during the `post-install-commands` will be handled correctly by `opam`.
files during the `post-install-commands` will be handled correctly by opam.

No monospace until you're talking about the opam file specifically. It sounds like this is talking about the pkg mgr in general. Is that correct?

Comment on lines 1412 to +1414
specify commands that will be run just after processing the package's commands
for the corresponding action, and the `<pkgname>.install` file in the case of
install and remove, on any package. The post commands are run wether or not
install and remove, on any package. The post commands are run whether or not
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do these items "specify commands that will be run...for the corresponding action" and specify commands for the "<pkgname>.install file (in the case of install and remove)"

Does the "on any package" part refer to both of the above (corresponding action and install file) or just one or the other?

Comment on lines +1376 to +1377
for context. If set to a single `<ident>` element that may point to a
built-in solver or one of the `aspcud`, `packup`, or `mccs` predefined
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has an If... but no then -- the thought is incomplete. If it's set to single <ident> ... then what happens?

`build`.

When the version formula reduces to `false`, as would be the case here when
`build=false`, the dependency is removed from the formula.

### Interpolation

Some files can be rewritten using variable interpolation expansion: in cases
Some files can be rewritten using variable interpolation expansion. In cases
where this is available, when looking for `file` and `file.in` is found, any
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is "when looking for file and file.in is found" an example of "where this is available"?
If so, let's write it like this; "In cases where this is available, i.e., when looking for ..."

...or does this mean "in cases where this is available and when looking for..., any interpolations found are replaced..."?


This field follows the exact same format as `build:`. It is used to move
products of `build:` from the build directory to their final destination
under the current `prefix`, and do any required additional setup. Commands
in `install:` are executed sequentially, from the root of the source tree
under the current `prefix`; they do any required additional setup. Commands
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could we change "do any required..." to "perform any required..."?

@@ -994,7 +991,7 @@ files.
dependencies of the package toward packages external to the <span class="opam">opam</span>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be "the package's external dependencies"?
Do the external dependencies belong to the package?

explicitely. This can be affected by the
[solver criteria](#configfield-solver-criteria). This can be useful for
beta releases, or to discourage installation of releases with known bugs.
beta releases or to discourage installation of releases with known bugs.

Note that this behaviour is disabled when a flagged version of the package
is already installed.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the following lines be:

- <a id="opamflag-deprecated">`deprecated`</a>: this flag is equivalent to
  [`avoid-version`](#opamflag-avoid-version), except for the addition of a
  deprecation message after the package is installed. It should also be marked as
  deprecated in the solution shown to the user upon installation.

The way it reads now is a little confusing.

@@ -614,22 +611,22 @@ For example, a dependency on `"foo" { = version }` will require package `foo` at
the version defined by variable `version` (which is the version of the package
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sentence is quite confusing.

"For example, a dependency on "foo" { = version } will require package foo at
the version defined by variable version (which is the version of the package being defined)."

How can we clarify this further?
Maybe:

"For example, a dependency on "foo" { = version } will require package foo at
the version, which is defined by variable version (the package version being defined)."

What is meant by "at the version" --- is this in the command or in a directory? a file?

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

Successfully merging this pull request may close these issues.

None yet

2 participants