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

Add specification document style guide #2884

Merged
merged 29 commits into from
Apr 8, 2021

Conversation

henrikt-ma
Copy link
Collaborator

@henrikt-ma henrikt-ma commented Mar 3, 2021

Preparing a style guide for the specification document, according to phone meeting decision.

This is a prerequisite for fixing #2713: once we have a style guide to follow, we'll have to set up another PR for actually applying the style guide to fix #2713.

Setting up PR in Draft state, so that others can give early feedback. Please note that I still haven't even started on the part directly related to #2713.

@henrikt-ma
Copy link
Collaborator Author

I think the style guide draft now has enough content to begin the review process. I'll leave the PR in Draft state to reflect that it's still in a very early stage of development.

Copy link
Member

@eshmoylova eshmoylova left a comment

Choose a reason for hiding this comment

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

Looks good. One thing I would like to see is a link to preamble.tex saying where to find various rules like nonnormative, example, definition, etc. Alternatively, we can list all of them in styleguide.md but that would require maintaining them in 2 places. It looks like they are documented relatively well in preamble.tex, so I think it would be enough to mention it and provide a link.

chapters/inheritance.tex Outdated Show resolved Hide resolved
styleguide.md Outdated Show resolved Hide resolved
styleguide.md Outdated Show resolved Hide resolved
styleguide.md Outdated Show resolved Hide resolved
styleguide.md Outdated Show resolved Hide resolved
@HansOlsson
Copy link
Collaborator

Looks good apart from the minor things I noticed.

styleguide.md Outdated Show resolved Hide resolved
@henrikt-ma
Copy link
Collaborator Author

Looks good. One thing I would like to see is a link to preamble.tex saying where to find various rules like nonnormative, example, definition, etc. Alternatively, we can list all of them in styleguide.md but that would require maintaining them in 2 places. It looks like they are documented relatively well in preamble.tex, so I think it would be enough to mention it and provide a link.

Yes, it would definitely be good if we could avoid any extra maintenance. I started by just mentioning the two files where we keep our macros and environments. With that, are links really necessary for each specific case? If anything, I think we should just give the name of the package where a command is defined, but I see a risk of bloating the style guide with package references, when it's actually rather easy to figure out once one knows where to look for the things we have defined, the other things being found by a search on CTAN or similar.

Copy link
Member

@eshmoylova eshmoylova left a comment

Choose a reason for hiding this comment

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

Looks good so far.

@henrikt-ma
Copy link
Collaborator Author

Looks good so far.

Then, with @HansOlsson's approval I suggest that the Draft state is removed to indicate that this could very well be pretty much ready for merging. Then, I'll also make an announcement on the mailing list, so that we can get additional comments well in advance of the next phone meeting. Hopefully, we'll then be ready for polling on accepting this at the meeting – @HansOlsson, could you please make sure it's on the agenda?

@HansOlsson HansOlsson added this to the Phone2021-2 milestone Mar 9, 2021
styleguide.md Outdated Show resolved Hide resolved
Reverting for two reasons:
- Functionality isn't working well, with two out of six cases showing broken highlighting.
- No way to get matching syntax highlighting in inline code snippets, making inline code and code blocks looks like two different kinds of code.

This reverts commit ecf28df.
@henrikt-ma
Copy link
Collaborator Author

With nothing said so far in favor of keeping the syntax highlighting for LaTeX code blocks, I reverted this part too.

@HansOlsson
Copy link
Collaborator

Some comments:

  • I'm not convinced that hyphen in DynamicSelect-expression and curve-expression is the right approach. I don't see a clear separation explaining why it is "DynamicSelect-expression" and "smooth operator". That we currently don't have it is another indication.

The rest are just attempts at improving the current texts:

  • Hard line breaks within paragraphs: I would think it a good idea to add: In case a sentence doesn't fit on one line on the screen a clearer alternative may be to break it into several sentences.
  • For start values it will in some places look odd that start isn't in Modelica, but other attributes like fixed are. I believe we should write "start attribute" in those cases.
  • The terminology for definitions seems odd. We have a definition environment (note: no hyphen); but if the definition is delayed we use willintroduce to indicate that it will be introduced later. To me it seems cleaner to have a "willdefine" macro, and say that it will be defined later.
  • The "Use of emph and italics" says that "Italics is used when new ...", but it isn't fully clear that this is internal to firstuse and that it uses emph.
  • Modelica Listings: At the end it says "Use hard line breaks and manual indentation of continued lines to meet this requirement." I would prefer: "Use hard line breaks and manual additional indentation of continued lines to meet this requirement. A semicolon in a matrix should either be followed by a line-break or by a space."

As suggested in review comment by Hans.
@henrikt-ma
Copy link
Collaborator Author

  • Modelica Listings: At the end it says "Use hard line breaks and manual indentation of continued lines to meet this requirement." I would prefer: "Use hard line breaks and manual additional indentation of continued lines to meet this requirement. A semicolon in a matrix should either be followed by a line-break or by a space."

Done.

@henrikt-ma
Copy link
Collaborator Author

henrikt-ma commented Mar 18, 2021

  • The "Use of emph and italics" says that "Italics is used when new ...", but it isn't fully clear that this is internal to firstuse and that it uses emph.

Done, without going into detail of how it is done inside the semantic macros.

@henrikt-ma
Copy link
Collaborator Author

  • Hard line breaks within paragraphs: I would think it a good idea to add: In case a sentence doesn't fit on one line on the screen a clearer alternative may be to break it into several sentences.

I think one should be careful about making such suggestions, as the priority should be readability of the document. Sure, long sentences can be its own source of poor readability, but a paragraph with too many and overly aggressively shortened sentences can also become hard to follow. For example, references such as "This…" can become ambiguous when following many short sentences, whereas ", this…" is clearly a reference to something within the same sentence.

Anyway, I tried adding something in the form of a rather weak suggestion.

@henrikt-ma
Copy link
Collaborator Author

  • For start values it will in some places look odd that start isn't in Modelica, but other attributes like fixed are. I believe we should write "start attribute" in those cases.

This form should only be used when the start attribute is not what is meant, so by definition, I don't think writing "start attribute" would be the right solution. I'd say it still remains to examine in detail exactly which variants there are in the text, and what the exact meaning of "start value" should really be, if any. I suggest that we return to this when actually trying to apply the style guide. If the rule doesn't make sense then, we can just update the style guide as needed on the fly.

@henrikt-ma
Copy link
Collaborator Author

henrikt-ma commented Mar 18, 2021

  • The terminology for definitions seems odd. We have a definition environment (note: no hyphen); but if the definition is delayed we use willintroduce to indicate that it will be introduced later. To me it seems cleaner to have a "willdefine" macro, and say that it will be defined later.

The terminology was based on a distinction between giving a full-blown definition, and just introducing a term in the running text. The use of \willintroduce that I had in mind was clearly that it would be followed by the latter, not a full-blown definition. Of course, this would have made more sense if \firstuse was renamed \introduce

Since introducing terms in running text is far more common than using a definition environment, I'm tempted to say with \willintroduce, but rename \firstuse accordingly. On the other hand, the name \firstuse works rather well as a reminder that one should try to introduce a term on its first use. Thoughts?

@henrikt-ma
Copy link
Collaborator Author

  • I'm not convinced that hyphen in DynamicSelect-expression and curve-expression is the right approach. I don't see a clear separation explaining why it is "DynamicSelect-expression" and "smooth operator". That we currently don't have it is another indication.

The reason was that I wanted "DynamicSelect expression" to mean an expression of type DynamicSelect, following a general rule about " expression". The meaning of "DynamicSelect-expression" is slightly different: an expression (annotation) in the form of a call to the (pseudo-code) record constructor DynamicSelect.

Since the need for such distinction is unique to "expression", we have the freedom to drop the hyphen in other cases, which looks better without sacrificing precision, in my opinion.

@HansOlsson
Copy link
Collaborator

  • I'm not convinced that hyphen in DynamicSelect-expression and curve-expression is the right approach. I don't see a clear separation explaining why it is "DynamicSelect-expression" and "smooth operator". That we currently don't have it is another indication.

The reason was that I wanted "DynamicSelect expression" to mean an expression of type DynamicSelect, following a general rule about " expression". The meaning of "DynamicSelect-expression" is slightly different: an expression (annotation) in the form of a call to the (pseudo-code) record constructor DynamicSelect.

Since the need for such distinction is unique to "expression", we have the freedom to drop the hyphen in other cases, which looks better without sacrificing precision, in my opinion.

I don't think the added perception of precision is worth it.

@HansOlsson
Copy link
Collaborator

An additional point found while reviewing another IR is that we generally don't use contractions in the text. That rule is missing from the style guide.

Includes addressing review comment by Hans regarding contractions.
@henrikt-ma
Copy link
Collaborator Author

An additional point found while reviewing another IR is that we generally don't use contractions in the text. That rule is missing from the style guide.

Done. Also specified that the document uses American English.

@henrikt-ma
Copy link
Collaborator Author

henrikt-ma commented Mar 20, 2021

I don't think the added perception of precision is worth it.

OK. Maybe a better solution is to suggest avoiding the possibly ambiguous combinations with expression? For example, it could be sufficient to just say:

… the correspondingCurve

instead of

… the correspondingCurve expression…

Or saying something like

The mandatory \lstinline!x! and \lstinline!y! settings

instead of

The mandatory \lstinline!x! and \lstinline!y! expressions

Other ways of rewriting can probably be found in the other cases as well.

@HansOlsson
Copy link
Collaborator

I don't think the added perception of precision is worth it.

OK. Maybe a better solution is to suggest avoiding the possibly ambiguous combinations with expression? For example, it could be sufficient to just say:

… the correspondingCurve

instead of

… the correspondingCurve expression…

Or saying something like

The mandatory \lstinline!x! and \lstinline!y! settings

instead of

The mandatory \lstinline!x! and \lstinline!y! expressions

Other ways of rewriting can probably be found in the other cases as well.

Yes.

And specifically for DynamicSelect:
Any value (coordinates, color, text, etc.) in graphical annotations can be dependent on class variables
using the DynamicSelect expression. DynamicSelect has the syntax of a function call with two arguments, where the first argument specifies the value of the editing state and the second argument the value of the non-editing state.
->
Any value (coordinates, color, text, etc.) in graphical annotations can be dependent on class variables
using DynamicSelect. DynamicSelect has the syntax of a function call with two arguments, where the first argument specifies the value of the editing state and the second argument the value of the non-editing state.

(At first I wanted to add 'call', but I found it redundant.)

@henrikt-ma
Copy link
Collaborator Author

Yes.

And specifically for DynamicSelect:
Any value (coordinates, color, text, etc.) in graphical annotations can be dependent on class variables
using the DynamicSelect expression. DynamicSelect has the syntax of a function call with two arguments, where the first argument specifies the value of the editing state and the second argument the value of the non-editing state.
->
Any value (coordinates, color, text, etc.) in graphical annotations can be dependent on class variables
using DynamicSelect. DynamicSelect has the syntax of a function call with two arguments, where the first argument specifies the value of the editing state and the second argument the value of the non-editing state.

(At first I wanted to add 'call', but I found it redundant.)

Done.

Copy link
Collaborator

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

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

Ok, looks good now.

@HansOlsson HansOlsson merged commit f999d1d into modelica:master Apr 8, 2021
@henrikt-ma henrikt-ma deleted the cleanup/style-guide branch April 9, 2021 23:52
@HansOlsson HansOlsson added the M36 For pull requests merged into Modelica 3.6 label Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M36 For pull requests merged into Modelica 3.6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unusual way of marking language terms using hyphen instead of \lstinline
4 participants