tuxji commented on PR #875: URL: https://github.com/apache/daffodil/pull/875#issuecomment-1314262592
> Do you want the specific class names and which methods were updated, just the class names that need updated/removed or just the file name? Shane, the best level of detail to put in a pull request is somewhat subjective. I personally review my own pull requests one file at a time and write a brief message about each file because doing so forces me to think about why the PR changes each file and sometimes reminds me of something I may have overlooked. Here is an example I see in your own pull request. The first changed file is Main.scala so I would write something like... `Main.scala: Call DataProcessor methods instead of deprecated HasSetDebugger methods.` I see two issues immediately when I look at Main.scala. The first issue is that when I search all the files changed for other mentions of HasSetDebugger, I don't see HasSetDebugger's definition, only that it is no longer one of DataProcessor's traits. I know immediately that the PR has not removed HasSetDebugger's definition yet (FYI, you can find HasSetDebugger's definition in the file daffodil-lib/src/main/scala/org/apache/daffodil/processors/Runtime.scala). The second issue I see is that you can't simply replace `setDebugg*` calls with `withDebugg*` calls. The `setDebugg*` calls are mutative - they change the called object's state. The `withDebugg*` calls are immutative - they return a copy of the called object with the new value instead of the old value, but the called object's state hasn't changed in any way. This could be why your integration tests are failing. This example shows how the very act of writing down why your PR changes each file can help you notice problems yourself. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
