Skip to content

Document property hierarchy.#9042

Closed
paulidale wants to merge 1 commit intoopenssl:masterfrom
paulidale:property-doc-levels
Closed

Document property hierarchy.#9042
paulidale wants to merge 1 commit intoopenssl:masterfrom
paulidale:property-doc-levels

Conversation

@paulidale
Copy link
Contributor

Add documentation to indicate the interaction between global (context level)
property queries and local (passed to fetch) ones.

  • documentation is added or updated

@paulidale
Copy link
Contributor Author

A very rough draft only.

Refer #9011

@paulidale paulidale mentioned this pull request May 30, 2019
1 task
Add documentation to indicate the interaction between global (context level)
property queries and local (passed to fetch) ones.
@paulidale paulidale force-pushed the property-doc-levels branch from a0bcb20 to fd1c979 Compare June 5, 2019 04:28
@paulidale
Copy link
Contributor Author

paulidale commented Jun 5, 2019

Merged, thanks @levitte

(after a rebase and squash)

@paulidale paulidale closed this Jun 5, 2019
levitte pushed a commit that referenced this pull request Jun 5, 2019
Add documentation to indicate the interaction between global (context level)
property queries and local (passed to fetch) ones.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9042)
@paulidale paulidale deleted the property-doc-levels branch June 5, 2019 04:31
@mattcaswell
Copy link
Member

This seems to have caused make doc-nits to fail, which is causing all travis jobs to fail.

The full syntax for property queries appears below, but the available syntatic
features are:

=over
Copy link
Member

Choose a reason for hiding this comment

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

=over 4

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 was fixed in #9087

Copy link
Contributor

@mspncp mspncp left a comment

Choose a reason for hiding this comment

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

Sorry for being late to the game, but since the nit's need to be fixed anyway, you might as well take a look at my review comments.

A context based property query that applies to all fetch operations and a local
property query.
Where both the context and local queries include a clause with the same name,
the local clause is used and the context one ignored.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to change this sentence to

Where both the context and local queries include a clause with the same name,
the local clause overrides the context clause.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

the local clause is used and the context one ignored.
For example, a context property query of "fips=yes" and a local property query
of "fips=no" would result in algorithms that have the "fips" property set t
"no".
Copy link
Contributor

Choose a reason for hiding this comment

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

and to omit this sentence, because it does not really add anything to the previous.

(If you prefer to keep the sentence, then a typo needs to be fixed: set t "no" - set to "no")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

=head2 Override

It is possible for a local property query to override a clause in the context
property query by preceeding the property name with a '-'.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not call this an override, it's more like erasing/removing the context clause.

Also, using the - prefix does not seem very intuitive to me. The -fips clause could easily be misinterpreted as "disable fips" instead of "I don't care about fips". How about adding a new special keyword instead, something like fips=any, fips=dontcare or fips=undef?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it to remove.

I'm less sure about the fips=xxx suggestion. There is another prefix operator but no predefined values that do special things. Removing one prefix operator and putting a special value in its place seems to complicated things.


It is possible for a local property query to override a clause in the context
property query by preceeding the property name with a '-'.
For example, a conxtet property query that contains "fips=yes" would normally
Copy link
Contributor

Choose a reason for hiding this comment

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

conxtet -> context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

result in implementations that have "fips=yes".
However, if the setting of
the "fips" property is irrelevant to the operations being performed, the local
property query can include the clause "-fips".
Copy link
Contributor

Choose a reason for hiding this comment

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

fips=irrelevant would be a nice way to say it, too ;-)

property query can include the clause "-fips".
Note that the local property query could not use "fips=no" because that would
disallow any implementations with "fips=yes" rather than not caring about the
setting.
Copy link
Contributor

Choose a reason for hiding this comment

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

TL;DR: All the explanations could probably be shortened, if the - prefix would be replaced by a meaningful special keyword, see above.

@paulidale paulidale mentioned this pull request Jun 6, 2019
1 task
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.

5 participants