Feat/80 char line break #24

macintoshpie · 2021-02-08T19:35:25Z

Background

modelicafmt does not take line length into consideration. This would be helpful to prevent really long lines.

Changes

break on long lines (80+ chars). modelicafmt will now insert a newline (and indent one additional level relative to the parent line) if:
- the previous token is "breakable", including , | = | (
- the length of the terminal token + the length of its whitespace prefix + the number of chars already written to the line is greater than 80

The linebreak is "dumb" meaning that all it does is insert the newline and indentation. e.g. assume there's 80 chars before c

# before formatter
something(a,b,c,d,e,f,g)
# after formatter
something(a,b,
  c,d,e,f,g)

An ideal implementation (in my opinion) would probably do something like this:

# before formatter
something(a,b,c,d,e,f,g)
# after formatter
something(
  a,
  b,
  c,
  d,
  e,
  f,
  g,
)

There are also some edgecases it can't handle. E.g. if we have <insert 78 chars>=a.sub_component.y it will not break after the =, though we would probably expect that it would. We would like to consider a.sub_component.y a single "token"/"thing" but our current tokenizer does not currently (it considers them separate: "a", ".", "sub_component", ".", "y"). So it will write the "a", and by the time it wants to break (on the ".") we've passed our opportunity to break.

AntoineGautier · 2021-02-11T22:41:58Z

@macintoshpie This PR solves the most problematic issues with long annotations. I have a few remarks enumerated below by order of priority.

I would not use { as a breaking character: this is an array constructor that we would like to keep next to the enclosed constructing clause.

When considering the following formatting I wonder if we should not break lines only if the annotation exceeds 80 characters (with a margin, say 5-10 characters). The reason is that the model developer (or the development tool) may have organized properly the annotation, which the fomater will ruin, producing less readable code.

// Before formatter
partial model PartialTwoPort "Partial component with two ports"
  replaceable package Medium =
    Modelica.Media.Interfaces.PartialMedium "Medium in the component"
      annotation (choices(
        choice(redeclare package Medium = Buildings.Media.Air "Moist air"),
        choice(redeclare package Medium = Buildings.Media.Water "Water"),
        choice(redeclare package Medium =
            Buildings.Media.Antifreeze.PropyleneGlycolWater (
              property_T=293.15,
              X_a=0.40)
              "Propylene glycol water, 40% mass fraction")));

// After formatter
partial model PartialTwoPort
  "Partial component with two ports"
  replaceable package Medium=Modelica.Media.Interfaces.PartialMedium
    "Medium in the component"
    annotation (choices(choice(redeclare package Medium=Buildings.Media.Air "Moist air"),
      choice(redeclare package Medium=Buildings.Media.Water "Water"),choice(
      redeclare package Medium=Buildings.Media.Antifreeze.PropyleneGlycolWater(
      property_T=293.15,X_a=0.40) "Propylene glycol water, 40% mass fraction")));

// Before formatter
connector ModeTypeOutput = output CHPs.BaseClasses.Types.Mode
  "Output connector for mode type"
annotation (
  defaultComponentName="y",
  Icon(
    coordinateSystem(preserveAspectRatio=true,
      extent={{-100.0,-100.0},{100.0,100.0}}),
      graphics={
    Polygon(
      lineColor={0,127,0},
      fillColor={255,255,255},
      fillPattern=FillPattern.Solid,
      points={{-100.0,100.0},{100.0,0.0},{-100.0,-100.0}})}),
  Diagram(
    coordinateSystem(preserveAspectRatio=true,
      extent={{-100.0,-100.0},{100.0,100.0}}),
      graphics={
    Polygon(
      lineColor={0,127,0},
      fillColor={255,255,255},
      fillPattern=FillPattern.Solid,
      points={{-100.0,50.0},{0.0,0.0},{-100.0,-50.0}}),
    Text(
      lineColor={0,0,127},
      extent={{30.0,60.0},{30.0,110.0}},
      textString="%name")}),
  Documentation(info="<html>

// After formatter
connector ModeTypeOutput=output CHPs.BaseClasses.Types.Mode
  "Output connector for mode type"
  annotation (defaultComponentName="y",Icon(coordinateSystem(preserveAspectRatio=
    true,extent={{-100.0,-100.0},{100.0,100.0}}),graphics={Polygon(lineColor={0,
    127,0},fillColor={255,255,255},fillPattern=FillPattern.Solid,points={{-100.0,
    100.0},{100.0,0.0},{-100.0,-100.0}})}),Diagram(coordinateSystem(
    preserveAspectRatio=true,extent={{-100.0,-100.0},{100.0,100.0}}),graphics={
    Polygon(lineColor={0,127,0},fillColor={255,255,255},fillPattern=FillPattern.Solid,
    points={{-100.0,50.0},{0.0,0.0},{-100.0,-50.0}}),Text(lineColor={0,0,127},
    extent={{30.0,60.0},{30.0,110.0}},textString="%name")}),Documentation(info=
    "<html>

When formatting Buildings/Fluid/Types.mo

// Before formatter
type CvTypes = enumeration(
  OpPoint "flow coefficient defined by m_flow_nominal/sqrt(dp_nominal)",
  Kv "Kv (metric) flow coefficient",
  Cv "Cv (US) flow coefficient",
  Av "Av (metric) flow coefficient")

// After formatter
type CvTypes=enumeration(OpPoint
  "flow coefficient defined by m_flow_nominal/sqrt(dp_nominal)",Kv
  "Kv (metric) flow coefficient",Cv
  "Cv (US) flow coefficient",Av
  "Av (metric) flow coefficient")

Why is it treated differently than a standard declaration with line break after (?

Another variant of that behavior, which in addition leads to a line exceeding 80 characters.

// Before formatter
record Viessmann_BW301A29_37kW_6_0COP_R410A =
  Buildings.Fluid.HeatPumps.Data.ScrollWaterToWater.Generic (
    volRat = 2.17993091357,
    V_flow_nominal = 0.00519094827579,
    leaCoe = 0.00125206659019,
    etaEle = 0.796906164099,
    PLos = 99.9278169331,
    dTSup = 0.532774492232,
    UACon = 27989.764814,
    UAEva = 58762.7503506)

// After formatter
record Viessmann_BW301A29_37kW_6_0COP_R410A=Buildings.Fluid.HeatPumps.Data.ScrollWaterToWater.Generic(
  volRat=2.17993091357,
  V_flow_nominal=0.00519094827579,
  leaCoe=0.00125206659019,
  etaEle=0.796906164099,
  PLos=99.9278169331,
  dTSup=0.532774492232,
  UACon=27989.764814,
  UAEva=58762.7503506)

Default to no max line length

macintoshpie · 2021-02-17T22:44:00Z

modelicafmt.go

+		"=",
+		",",
+		"(",


tokens we allow line breaks on (when > max chars per line)

nice, seems like it would be easy to add others as needed!

macintoshpie · 2021-02-17T22:45:04Z

main.go

@@ -18,6 +18,7 @@ import (
 var (
 	write       = flag.Bool("w", false, "overwrite the file(s)")
 	versionFlag = flag.Bool("v", false, "display tool version")
+	lineLength  = flag.Int("line-length", -1, "how many characters allowed per line; -1 means no max")


new cli arg, defaults to no max line length

verrry noice

macintoshpie · 2021-02-17T22:47:29Z

@AntoineGautier Thanks for the feedback, I removed the curly right brace as a possible char to break after. Unfortunately your other request to preserve original whitespace is not addressable currently

@nllong let me know if you have any other thoughts or preferences on this.

nllong

looks swell!

nllong · 2021-02-19T16:23:53Z

main.go

@@ -18,6 +18,7 @@ import (
 var (
 	write       = flag.Bool("w", false, "overwrite the file(s)")
 	versionFlag = flag.Bool("v", false, "display tool version")
+	lineLength  = flag.Int("line-length", -1, "how many characters allowed per line; -1 means no max")


verrry noice

nllong · 2021-02-19T16:25:04Z

@macintoshpie You want to merge and bump release version?

macintoshpie added 2 commits February 8, 2021 12:27

feat: add condition to break long lines, 80+ chars

16c5421

test: apply updated formatter to test files

be06ff1

macintoshpie mentioned this pull request Feb 8, 2021

Run modelica-fmt v0.2-pr.1 AntoineGautier/modelica-buildings#2

Closed

macintoshpie added 4 commits February 15, 2021 14:04

refactor: don't allow line break on curly brace

314849f

test: update gmt building out file

7375e5b

feat(cli): add flag for line length

e52f7ca

Default to no max line length

test: update tests and output for max line length

af8e216

macintoshpie requested a review from nllong February 17, 2021 22:43

macintoshpie commented Feb 17, 2021

View reviewed changes

macintoshpie marked this pull request as ready for review February 17, 2021 22:47

nllong approved these changes Feb 19, 2021

View reviewed changes

macintoshpie merged commit 083ec2b into develop Feb 19, 2021

macintoshpie deleted the feat/80-char-line-break branch February 19, 2021 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/80 char line break #24

Feat/80 char line break #24

macintoshpie commented Feb 8, 2021 •

edited

Loading

AntoineGautier commented Feb 11, 2021

macintoshpie Feb 17, 2021

nllong Feb 19, 2021

macintoshpie Feb 17, 2021

nllong Feb 19, 2021

macintoshpie commented Feb 17, 2021

nllong left a comment

nllong Feb 19, 2021

nllong commented Feb 19, 2021

Feat/80 char line break #24

Feat/80 char line break #24

Conversation

macintoshpie commented Feb 8, 2021 • edited Loading

Background

Changes

AntoineGautier commented Feb 11, 2021

macintoshpie Feb 17, 2021

Choose a reason for hiding this comment

nllong Feb 19, 2021

Choose a reason for hiding this comment

macintoshpie Feb 17, 2021

Choose a reason for hiding this comment

nllong Feb 19, 2021

Choose a reason for hiding this comment

macintoshpie commented Feb 17, 2021

nllong left a comment

Choose a reason for hiding this comment

nllong Feb 19, 2021

Choose a reason for hiding this comment

nllong commented Feb 19, 2021

macintoshpie commented Feb 8, 2021 •

edited

Loading