refactor(plugin-odap-hermes): rename package, variables ODAP -> SATP

[AndreAugusto11]

• renamed every variable from odap to satp
• left out CHANGELOG files

Closes #2809

cc @RafaelAPB

[petermetz]

@AndreAugusto11 LGTM with comment(s)

1. Please update the commit subject line to go like the string I just set the PR title to (refactor(plugin-odap-hermes): rename package, variables ODAP -> SATP) this way it's clearer which specific plugin had the renaming done and also that it's not a new feature just a refactor of an existing feature. The reason why I pick on this is the generated release notes which would include it as a new feature but we had it for quite a while!

[AndreAugusto11]

@AndreAugusto11 LGTM with comment(s)

1. Please update the commit subject line to go like the string I just set the PR title to (refactor(plugin-odap-hermes): rename package, variables ODAP -> SATP) this way it's clearer which specific plugin had the renaming done and also that it's not a new feature just a refactor of an existing feature. The reason why I pick on this is the generated release notes which would include it as a new feature but we had it for quite a while!

@petermetz, that makes much sense. Fixed! Thanks

[AndreAugusto11]

Note: an important takeaway from this PR is that variable names in a package should not be so dependent on the name of the package (e.g., instead of calling a log IOdapLocalLog, just call it ILocalLog, given that it is already being defined within the plugin-odap-hermes package). It would have simplified this PR and might be helpful in future refactors.

[petermetz]

Note: an important takeaway from this PR is that variable names in a package should not be so dependent on the name of the package (e.g., instead of calling a log IOdapLocalLog, just call it ILocalLog, given that it is already being defined within the plugin-odap-hermes package). It would have simplified this PR and might be helpful in future refactors.

@AndreAugusto11 I agree with what you said, but I can also see why the higher specificity of the naming can help in situations where the code imports from multiple different plugins and then there would be multiple different ILocalLog classes to juggle (which would force you to rename them upon import). On the other hand the renaming refactor is probably something that happens not that often so having a better developer experience for the day to day use with higher specificity might be the way to go. So in the end my take is that it just depends. :-)

Note for future: Don't forget to re-request review. I'm only seeing this now because I was going through older messages but it wasn't (still isn't) in my PR review queue.

[petermetz]

LGTM

[outSH]

@AndreAugusto11 Looks good but fix required tests yarn_lint and yarn_codegen. In a former you probably need to add exceptions for Satp, satp etc.. in .cspell files, for latter rebase with main and run codegen.

[AndreAugusto11]

Note: an important takeaway from this PR is that variable names in a package should not be so dependent on the name of the package (e.g., instead of calling a log IOdapLocalLog, just call it ILocalLog, given that it is already being defined within the plugin-odap-hermes package). It would have simplified this PR and might be helpful in future refactors.

@AndreAugusto11 I agree with what you said, but I can also see why the higher specificity of the naming can help in situations where the code imports from multiple different plugins and then there would be multiple different ILocalLog classes to juggle (which would force you to rename them upon import). On the other hand the renaming refactor is probably something that happens not that often so having a better developer experience for the day to day use with higher specificity might be the way to go. So in the end my take is that it just depends. :-)

Note for future: Don't forget to re-request review. I'm only seeing this now because I was going through older messages but it wasn't (still isn't) in my PR review queue.

Have a good point there! This is probably more suited to local variable names in function arguments, test files, etc...

[AndreAugusto11]

@petermetz @outSH thank you both for your comments!!

[petermetz]

@AndreAugusto11 Plus one to @outSH 's request above! Otherwise LGTM!

Error: yarn codegen script produced version control side-effects: source files have been changed by it that are otherwise are under version control. This means (99% of the time) that you need to run the yarn codegen script locally and then include the changes it makes in your own commit when submitting your pull request.

[petermetz]

@AndreAugusto11 Thank you for fixing the codegen check! LGTM