BrianGarland / vscode-rpgfree

Visual Studio Code extension to convert fixed format RPGLE to free format
MIT License
18 stars 11 forks source link

JS fixes/modernization/modularisation #75

Closed stackmystack closed 11 months ago

stackmystack commented 1 year ago

The purpose of this PR is to:

  1. Build on top of the test-suite PR
  2. Refactor the code base because it suffers from many issues:
    1. Incorrect use of string literals and string interpolation.
    2. Incorrect use of == and !=.
    3. Incorrect use of String APIs, notably the use of the substr deprecated call.
    4. Inconsistency in the use of curly braces with if-else clauses, to the point where long if branches would have curly braces, and their terminal else branch would not; it's not only bad taste, but also the liberal use of curly braces makes it harder to read and understand code. I would argue for the consistent use of curly braces even for one-liners:
      1. One less rule to keep in mind
      2. No room for ambiguity
      3. Limits of blocks of code are easily located at a glance, no extra effort required
    5. Breaking naming conventions: "globals" where in PascalCase instead of the JS norm of camelCase
    6. Liberal use of let declarations, so const was declared when necessary.
    7. The use of CommonJS modules which are horrible to look at an maintain, not to mention that they're not standard, unlike ES modules.
    8. Trailing white spaces everywhere which makes diffs and collaborative work a headache + lack of automatic code formatting
    9. Repetitive calls to toUpperCase

I would say that a lot of refactoring can still be done, but this is a good point.

stackmystack commented 1 year ago

It goes without saying that this should be inspected/reviewed/merged once the test-suite PR gets merged.

BrianGodsend commented 1 year ago

@stackmystack,

I cloned the main branch to fix some of the issues I found and/or were previously reported. Now, looking more closely at the state of this project, perhaps I should have started with your fork?

I am very unfamiliar with GitHub and the proper use of clone, branch, fork, commit, etc. This is easily seen by the silly commits and recommits I did in trying to link my commits messages to issues (which I think I got right the first time!). That said, I am wondering if it would have been better for me to start with your fork, as you likely already fixed most of the issues I worked on.

Please let me know your thoughts. Assuming you think your version is stable enough and open to letting others tweak your code (if needed), I guess the first steps would be to clone your fork and test it against the issues I think I resolved. If there is a problem, I would want your guidance on how to properly commit any changes.

stackmystack commented 11 months ago

@BrianGodsend I'm sorry I haven't seen any notification regarding this thread. Please excuse this very very late reply.

I am very unfamiliar with GitHub and the proper use of clone, branch, fork, commit, etc. …

Git and Github are not straighforward, and they have a huge learning curve.

Please let me know your thoughts.

My interest in this project stemmed from work-related stuff, and honestly, this came to the bottom of our priorities, and I would say I might never come back to it. I am no RPGLE expert.

I think that the changes here are needed for the project, as to whether to merge them before any changes you made, I can't tell because I didn't look at your work.