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

Update .rpt text to indicate OWA SWMM codebase #365

Closed
jennwuu opened this issue Jan 18, 2022 · 14 comments
Closed

Update .rpt text to indicate OWA SWMM codebase #365

jennwuu opened this issue Jan 18, 2022 · 14 comments

Comments

@jennwuu
Copy link

jennwuu commented Jan 18, 2022

Related to discussion from #352

It is important user understands the source of model results [re-replicating results, documentation, etc]. When a user views .rpt, the header is not descriptive to indicate which source of engine used. I think this is important as we add more features in the api.

At the moment, the .rpt file shows:

  EPA STORM WATER MANAGEMENT MODEL - VERSION 5.1 (Build 5.1.14)
  --------------------------------------------------------------
@jennwuu
Copy link
Author

jennwuu commented Jan 18, 2022

We should close this issue in the next release since #352 is closed.

Can we decide on the header text for the .rpt file?

@bemcdonnell @michaeltryby @cbuahin @abhiramm7

@bemcdonnell
Copy link
Member

@jennwuu and @michaeltryby, How about this?

  EPA STORM WATER MANAGEMENT MODEL  (OWA-Build 5.1.14)
  Build: asdfn92hf98haf9a8hf43
  --------------------------------------------------------------

@michaeltryby
Copy link

michaeltryby commented Jan 18, 2022

This is what is in there now. It seems fine.

report_writeLogo()
...
OWA STORM WATER MANAGEMENT MODEL - VERSION v%s (Build %.10s)"

I don't think we should add another line to the header as that may throw off someone parsing the report.

@bemcdonnell
Copy link
Member

@michaeltryby I agree. adding the SHA is probably redundant.

@jennwuu
Copy link
Author

jennwuu commented Jan 18, 2022

@michaeltryby PCSWMM parses text between brackets to relay build information from .rpt. Can we add OWA in the bracket to something like this?

OWA STORM WATER MANAGEMENT MODEL - VERSION v%s (OWA Build %.10s)

@bemcdonnell
Copy link
Member

@michaeltryby i think we are converging

...
OWA STORM WATER MANAGEMENT MODEL - VERSION v%s (OWA Build v%s.%.10s)"

@michaeltryby
Copy link

We are moving to semantic version standard - major, minor, patch. Previously, patch was being referred to as build but technically that's not accurate. In the version header I created BUILD_ID is a time stamp.

@bemcdonnell
Copy link
Member

so @michaeltryby, for example....

OWA STORM WATER MANAGEMENT MODEL - VERSION (OWA 6.1.0)

@michaeltryby
Copy link

Why does OWA need to be in there twice? In case they don't see it the first time?

@jennwuu
Copy link
Author

jennwuu commented Jan 18, 2022

@michaeltryby PCSWMM parses text between brackets to relay build information from .rpt. Can we include additional OWA text in the bracket?

@bemcdonnell
Copy link
Member

@jennwuu,

Let's go with your choice for now:

OWA STORM WATER MANAGEMENT MODEL - VERSION 5.1 (OWA 5.1.14)

@michaeltryby
Copy link

We have new variables in the swmm.dll that encode more version and build information than ever before. We should get away from parsing the report file for information and create and use APIs.

@bemcdonnell
Copy link
Member

@michaeltryby, we can circle back before the next release.

jennwuu added a commit to jennwuu/Stormwater-Management-Model that referenced this issue Jan 24, 2022
@abhiramm7
Copy link
Collaborator

This issue is resolved in #372

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

No branches or pull requests

4 participants