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

Formatting Script #9153

Merged
merged 9 commits into from
Oct 12, 2024
Merged

Formatting Script #9153

merged 9 commits into from
Oct 12, 2024

Conversation

N-thony
Copy link
Collaborator

@N-thony N-thony commented Sep 25, 2024

fixes partly #8847
@rdstern @derekagorhom have a look of the formatting of the script.
image
image

@derekagorhom
Copy link
Contributor

@N-thony I tested this PR and it works, it also fixes the other issues with regards to my PR.
@rdstern maybe able to give a more detailed review than me

@rdstern
Copy link
Collaborator

rdstern commented Sep 25, 2024

@derekagorhom I must be missing something? I loaded a script and pressed right-click and here is what I got:

image

I don't see any difference, or formatting options? What should I have done? Should I have installed a package?

@derekagorhom
Copy link
Contributor

derekagorhom commented Sep 25, 2024

@rdstern The reformat option is on the Edit Menu now

@N-thony
Copy link
Collaborator Author

N-thony commented Sep 25, 2024

@rdstern you have now Reformat Code in the right click menu. Of course, you need to select the text you want to reformat.
It works the same way with RStudio i.e selecting the script first then you go to Code > Reformat Code in RStudio
image

rdstern
rdstern previously approved these changes Sep 25, 2024
Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@N-thony that's great.
I like the right-click, and I like it being called Reformat Code.
Do we still want the Format Code in the edit menu? If so, should there be a line above and should it be Reformat Code for consistency with the right-click option?

I am approving while you consider this. @Patowhiz could you check, react and merge?

@N-thony
Copy link
Collaborator Author

N-thony commented Sep 26, 2024

@rdstern I removed format code menu

rdstern
rdstern previously approved these changes Sep 26, 2024
Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Patowhiz this should be an easy check and merge?

@@ -1025,4 +1068,7 @@ Public Class ucrScript
sender.Dispose()
End Sub

Private Sub mnuReformatCode_Click(sender As Object, e As EventArgs) Handles mnuReformatCode.Click
FormatRCode()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@N-thony if FormatRCode() is used once, then just put it's functionality inside the mnuReformatCode click event.
Same with EscapeDoubleQuotes() function. It's just one line and if used in one place only, just call the single line where it is used.
This will simplify the functionality added consequently making it easier to maintain.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

rdstern
rdstern previously approved these changes Sep 26, 2024
Dim formattedText As String = String.Join(Environment.NewLine, formattedCode)

' Check if there is any selected text
If clsScriptActive.SelectionStart <> clsScriptActive.SelectionEnd Then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@N-thony I don't quite understand the need for this check. Line 1023 Dim scriptText As String = clsScriptActive.SelectedText.Replace("""", "\""") alreadu assumes there is selected text. That's why it is formatting the it. Why do you need to check for selection again? I though if there is no selection, formatting shouldn't happen.
If there is a case when you are allowed to format nothing, shouldn't the test for selection be done before line 1023 is executed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't make sense to reformat nothing, I have made a change for an early sub exit when nothing is selected.
I hope this makes more sense, have a look

Patowhiz
Patowhiz previously approved these changes Sep 26, 2024
Copy link
Contributor

@Patowhiz Patowhiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rdstern the code now looks good. Kindly retest then I can merge.

@rdstern
Copy link
Collaborator

rdstern commented Sep 26, 2024

@N-thony please can you check for me. I am trying to check, while on a train, so may hace it weong. But when I select a part of the code and then right-click it seems to unselect it. So I haven't been able to reformat.
Is this a new problem in the code, or am I doing something wrong?

@N-thony
Copy link
Collaborator Author

N-thony commented Sep 26, 2024

@N-thony please can you check for me. I am trying to check, while on a train, so may hace it weong. But when I select a part of the code and then right-click, it seems to unselect it. So I haven't been able to reformat. Is this a new problem in the code, or am I doing something wrong?

@rdstern it is fine on my end
image
image

@N-thony
Copy link
Collaborator Author

N-thony commented Oct 11, 2024

@rdstern @Patowhiz I have resolved the conflicts

Copy link
Contributor

@Patowhiz Patowhiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rdstern over to you to review.

Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@N-thony and @Patowhiz that's very nice!

@Patowhiz Patowhiz merged commit d6705b3 into IDEMSInternational:master Oct 12, 2024
2 checks passed
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

Successfully merging this pull request may close these issues.

4 participants