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

Refactor description of arrays in debugger. #713

Closed
wants to merge 7 commits into from

Conversation

sampottinger
Copy link
Collaborator

Closes #606, builds on #692 from @WillRabalais04.

WillRabalais04 and others added 4 commits March 24, 2023 01:47
Updates the getStringValue method of the VariableNode class so that it correctly handles multidimensional arrays. The default formatting of multidimensional arrays is to have the size of the first array written in the last set of brackets eg.int[][5]. This changes it so that the returned value has the size of the first array written in the first set of brackets eg.int[5][]. This can parse multidimensional arrays of any size.
Closes Debugger lists immediate array dimension last #606.
@sampottinger
Copy link
Collaborator Author

sampottinger commented May 9, 2023

[edited slightly]

Hey @WillRabalais04! Can you do a code review on this and see how my additions feel? It’s built from your #692 (thank you! 👍✨) with a few additions:

  • The function was getting a bit nested so broke out a private method for the array case.
  • I like what you did with the regex but, as I was working with it, I noticed all of your logic could all fit into your regex itself, saving us a branch on if multi-dimensional array.
  • Added tests (use ant test)!

Re, regex: We can reverse out on this. Curious to get your opinion. Given the age and multiple strata from multiple generations of contributors (😅) I've found that, for Processing, sometimes consolidating to a documented / tested regex is sometimes a bit easier to maintain when working on some of the string manipulation if the regex isn’t too involved.

@sampottinger
Copy link
Collaborator Author

sampottinger commented May 9, 2023

CC @SableRaf - Is it possible to add GSoC folks to the repo if they join our communal pledge not to push direct to main? @WillRabalais04 - how would you feel about that power / responsibility?

@SableRaf
Copy link
Contributor

CC @SableRaf - Is it possible to add GSoC folks to the repo if they join our communal pledge not to push direct to main?

We could also consider adding branch protection to the repository: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches

@sampottinger
Copy link
Collaborator Author

Yeah I’m into that. I think we haven’t so far because the list of contributors with upstream access has been small. Requiring pr (with an exceptions list including Ben) to get to main would be ideal either way though.

@sampottinger
Copy link
Collaborator Author

Superseded by #729

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Debugger lists immediate array dimension last
3 participants