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

Start clearing up annotation syntax for functions. #3617

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

Conversation

HansOlsson
Copy link
Collaborator

@HansOlsson HansOlsson commented Nov 29, 2024

It is intended to work towards close #3495 - but it doesn't handle all of the annotations as some are problematic.

Note that I deliberately added default values in some cases, I believe they are the correct ones.

It might be that we should add more, or view this as enough to close it - and later think about the rest.

The problem with the other ones are:

  • "Trivial": the external function ones can specify either an array or a scalar.
  • "Messy": The derivative and inline-annotations don't fit into the pattern for two reasons:
    • The "value" is not a value - so no good type for the declaration (it is a function for derivatives, and something else for inverse)
    • The optional sub-modifiers for values, like smoothOrder(normallyConstant=x)=2

(I have might have missed some that could be handled.)

@eshmoylova
Copy link
Member

I updated the description to make it a link to the issue #3495 for an easy lookup.

@HansOlsson HansOlsson added this to the 2024-Decemeber milestone Dec 4, 2024
chapters/functions.tex Outdated Show resolved Hide resolved
Copy link
Collaborator

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

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

Suggesting minor change that will make this more compatible with #3621.

\begin{synopsis}[grammar]\begin{lstlisting}
"LateInline" "=" ( false | true )
\begin{synopsis}\begin{lstlisting}
/*literal*/ constant Boolean LateInline = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we decided to avoid writing out defaults like this, as it gives the impression that LateInline = false is what one is supposed to write?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I recall the idea was not use the form when specifying the default literal value (assuming it exists), when it is so "normal" that specifying it would look like an error (like singleInstance).

Different inlining variant and GenerateEvents don't fall under that; and is more similar to absoluteValue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's not how I recall it. For me, the main problem which was resolved when removing all those defaults was that what the reader sees first shouldn't be something one normally doesn't write when using the annotation. This was avoided by drastically reducing the use of declaration equations for explaining defaults in case of component style annotations.

I find LateInline similar to DocumentationClass, for which the default was removed in aa6ce47.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To me these two explanations are fairly similar.
We agree that using DocumentationClass = false does not really make sense at all in practice, the point is that LateInline = false is just a bit unusual - one can easily see someone experimenting with setting LateInline to true or false to test the result.

And most of the removed defaults in that commit weren't literal values, but explanations as defaults.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, so we have different recollections of the exact formulations used in the meeting discussions behind removing literal defaults. Fine.

Now, if we think about how to write clear specification text, don't you agree that there is a problem with picking the style of presentation based on vague opinions about what is more or less unusual or odd? … that specifying defaults for component style annotations sometimes with declaration equation and sometimes in text (even when the default corresponds to a literal value), is causing more confusion than useful brevity?

Copy link
Collaborator

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

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

I'm afraid defaults should be presented in the text rather than as a default binding.

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.

Notation used for function annotations will soon be obsolete
3 participants