innovyze / Open-Source-Support

This repository hosts open code that can be used in Innovyze products that support scripting. This includes Ruby for the UI/Exchange, SQL and other useful stuff. We stand on the shoulders of badgers and penguins.
44 stars 20 forks source link

Added line to replace Carriage Returns with newline #8

Open dirwin5 opened 2 years ago

dirwin5 commented 2 years ago

rtc_string in ICM v9.5.5 has carriage returns rather than newline characters. Added line to replace these.

sonarcloud[bot] commented 2 years ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

dfmore commented 2 years ago

Hi @dirwin5, thank you for this pull request. Could you give me an example of when the old syntax could be problematic in 9.5.5? I've just tried it in the new version of ICM and it seems to work as expected. Thanks

sancarn commented 2 years ago

Personally I would be against this PR. \r\n is typical for Windows OS, and editors such as notepad often require \r\n as a line ending. So personally I think ICM, as a Windows-first platform, and thus this script also should keep the \r\n line endings.

dirwin5 commented 2 years ago

Hi @dfmore, I've attached an image showing the file exported using the old syntax (left) vs rtc data manually exported from the ICM UI (right) using ICM v9.5.5. There are a lot of blank lines with the old syntax export due to the \r\n line endings.

Using the old syntax with newer versions of ICM, the export seems to follow the format on the right ok, as the exported data seems to have \n line endings rather than \r\n.

The screenshot shows the exported files opened using notepad on Windows 10.

Capture

sancarn commented 2 years ago

Mhm.... it could be that ruby replaces all \r and \n with \r\n thus resulting in \r\n ==> \r\n\r\n...

Looking up online it seems there is a line ending character $\ and it seems this is could be the reason why file.write isn't working. Supposedly using file.puts doesn't have this issue.

@dirwin5 can you check File.open(path + exported_file, 'w') { |file| file.puts(rtc_string) } instead of your check. Otherwise could you upload the 2 files to a GIST so we can inspect the line endings properly?

dirwin5 commented 1 year ago

Hi @sancarn and @dfmore, sorry for the delayed response, I've been away for a few days.

I tried replacing file.write with file.puts, however I found that this does not change the output file. There are still extra blank lines in the v9.5.5 export.

I tested adding p rtc_string after File.open(path + exported_file, 'w') { |file| file.puts(rtc_string) } to see the printed line endings clearly.

I believe ICM version 9.5.5 (and earlier) uses \r\n line endings, and this was changed in later versions to \n. My pull request would replace \r\n line endings with \n, which should fix the formatting of v9.5.5 exports, without affecting formatting of exports from later versions.

I have attached output files from both v9.5.5 and v2021.5.0 if you want to inspect the line endings. Note that these two exports are for different model networks so their content is different. Their formatting and line endings are visible however.

exported_rtc_v9.5.5.txt exported_rtc_v2021.5.0.txt

sancarn commented 1 year ago
  • In ICM version 9.5.5 the line this prints starts "!Version=1,type=RTC,charset=UTF8\r\n
  • whereas in ICM version 2021.5.0 the printed line starts "!Version=1,type=RTC,encoding=UTF8 \n

@dirwin5 So are you saying ICM version 2021.5.0 doesn't allow the import of files with \r\n line endings?

If so I'd say the bug here is really in ICM version 2021.5.0. The format for these files should remain static and the parser should remain backwards compatible. I feel the bug needs to be fixed in ICM itself and/or at least the code made to allow for either \r\n or \n.

@dfmore If you can find out what version this changed on, then we can change the script to do \r\n or \n dependent on ICM version. I feel that would be better, as otherwise currently the script will work for some people and not work for others...


EDIT:

I think I misunderstood again... You can't import/export RTC correct? And I'd imagine if you do row_object['rtc_data']=str then in ICM version 2021 it would require \r\n but in previous versions it would require \n. So really this has to do with an import target on the script... And/or changing the import script to handle this better. I believe replacing \r\n with \n isn't really the answer here.

@dfmore If you could get the version that this change occurred that'd be great, then we can essentially add:

if semver(Application.version) > "xxxx.xx.xxxx" 
  str.gsub(/\r?\n/,"\n")
end

to the import script.

dirwin5 commented 1 year ago

Hi @sancarn, this only seems to affect the exported rtc file when using the ruby script. Import seems to be fine with the original code.

The main reason I came across this issue is because I was using the ruby script to export rtc data from a model in ICM v9.5.5, and then using that rtc data in a separate external tool. There was a difference in file format which caused issues trying to read the data.

sancarn commented 1 year ago

@dirwin5 I see! Those images really explain the story much better! So essentially Ruby pre 2021 (or pre some version) errornously exported RTC data with \r\n when actually \n is needed when compared to Export > RTC data... spec. ICM post v2021.5 (or post someversion) has since fixed this issue, but this examples library should remain as agnostic as possible version wise. So the suggested change is to fix \r\n==>\n if it exists.

Whereas exporting RTC data from the ICM UI in v9.5.5 (Network > Export > RTC data...) gives the following file:

My concern is, if you try to import exported_rtc_v9.5.5_manual.rtc into ICM 9.5.5 with ruby, I.E:

network = WSApplication.current_network
row_object = network.row_object('hw_rtc',nil)  
rtcdata = File.read("path/to/exported_rtc_v9.5.5_manual.rtc")
network.transaction_begin
  row_object['rtc_data'] = rtcdata
  row_object.write
network.transaction_commit

Does this work as expected? Because if row_object['rtc_data'] is exporting with \r\n, i'd be concerned that row_object['rtc_data']= also requires \r\n line endings...

Alternatively you could test:

# Test me in ruby 9.5.5
network = WSApplication.current_network
row_object = network.row_object('hw_rtc',nil)  
network.transaction_begin
  row_object['rtc_data'] = row_object['rtc_data'].gsub(/\r\n/,"\n")
  row_object.write
network.transaction_commit
p row_object['rtc_data']

My concern/expectation is this might (if it works) accidentally keep \r characters resulting in \r\r\n... Or it will simply error.

Would you be able to test in 9.5.5? and perhaps include the result of the 2nd code (when ran in 9.5.5) in your response.


If \r\n is required for certain ICM versions then the ideal solution would be a larger change:

# Example 1: Export RTC from current network to a text file
if mode == 1
  #FIX: Ensure fix line endings to standard ICM spec. Force \r\n line endings to \n. See PR #8.
  File.write(path + exported_file, row_object['rtc_data'].gsub(/\r?\n/,"\n"))
  puts "File \'#{exported_file}\' exported to path \'#{path}\'"
end

# Example 2: Import RTC from a file into current network
if mode == 2
  imported_rtc = File.read(path + imported_file)
  network.transaction_begin
    #FIX: Version check before import and ensure line endings match spec for ruby at the time. See PR #8.
    row_object['rtc_data'] = import_rtc.gsub(/\r?\n/, WSApplication.version <= SOMEVERSION ? "\r\n" : "\n")
    row_object.write
  network.transaction_commit
  puts "RTC imported from file \'#{imported_file}\' on path \'#{path}\'"
end

(The above example is mostly unchanged from yours with a small change to mode 2)


Alternatively might be able to fix with begin/rescue depending on whether there are errors?

    #FIX: Version check before import and ensure line endings match spec for ruby at the time. See PR #8.
    begin
      row_object['rtc_data'] = import_rtc
    rescue # Attempt with \r\n line endings instead of \n
      row_object['rtc_data'] = import_rtc.gsub(/\r?\n/, "\r\n")
    end