Fix QS: Output Schema - static_strings json rendering

[Tzion0]

This changes resolves the static_strings JSON rendering issue specified by @mr-tz in #721.

I'm aware there’s more to do and will continue in the near future. Please correct me if I made any mistakes, as this is my first time contributing to this project <3

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[gemini-code-assist]

Hello @Tzion0, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

This pull request addresses issue #721 regarding the JSON rendering of static strings in the floss tool. The primary change involves disabling the sorting of keys in the JSON output to preserve the original order of the static_strings data. Additionally, the StaticString class in floss/results.py is enhanced with id and length attributes, and the string extraction functions in floss/strings.py are modified to populate these new attributes during string extraction. The extract_strings function in floss/utils.py is also updated to include the id and length attributes when yielding StaticString objects.

Highlights

• JSON Rendering: Disabled sorting of keys in JSON output to maintain the original order of static_strings.
• StaticString Class Enhancement: Added id and length attributes to the StaticString class in floss/results.py.
• String Extraction: Modified string extraction functions in floss/strings.py to populate the id and length attributes of StaticString objects.

Changelog

Click here to see the changelog

• floss/render/json.py
  • Disabled sorting of keys in the JSON output by changing sort_keys=True to sort_keys=False on line 43.
• floss/results.py
  • Added id and length attributes to the StaticString class on lines 141 and 145, respectively.
  • Added documentation for the id and length attributes to the StaticString class on lines 141 and 145, respectively.
  • Removed unnecessary whitespace and newlines in the StackString class documentation on lines 61, 62, 64, and 78.
• floss/strings.py
  • Modified extract_ascii_strings function to include id and length attributes when yielding StaticString objects on line 68.
  • Modified extract_unicode_strings function to include id and length attributes when yielding StaticString objects on line 96.
• floss/utils.py
  • Modified extract_strings function to include id and length attributes when yielding StaticString objects on line 300.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

---

Did you know?

The JSON format was derived from JavaScript but is language-independent and used by many programming languages.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request addresses the static_strings JSON rendering issue. The changes include disabling sorting of keys in the JSON output, adding id and length attributes to the StaticString class, and modifying the string extraction functions to include these new attributes. Overall, the changes seem reasonable and address the reported issue. However, there are a few areas that could be improved.

Summary of Findings

• Missing Documentation for ID and Length: The addition of id and length attributes to the StaticString class lacks sufficient documentation. While the attribute descriptions are present, there is no explanation of how these IDs are generated or the purpose of tracking string length. This could lead to confusion for future developers.
• Inconsistent Use of int(): The code uses int(i+1) to assign IDs to static strings. While this works, it might be more appropriate to use a dedicated counter or generator to ensure uniqueness and avoid potential issues if the extraction logic changes.

Merge Readiness

The pull request addresses the reported issue and introduces new functionality. However, the missing documentation and potential for ID collisions should be addressed before merging. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. I recommend that the pull request not be merged until the medium severity issues are addressed.

[gemini-code-assist]

Using int(i+1) to assign IDs might be problematic if the extraction logic changes or if there are multiple calls to these functions. Consider using a dedicated counter or generator to ensure uniqueness.

Suggested change

	for i, match in enumerate(r.finditer(buf)):
	yield StaticString(id=int(i+1), string=match.group().decode("ascii"), offset=match.start(), encoding=StringEncoding.ASCII, length=len(match.group().decode('ascii')))
	yield StaticString(id=int(i + 1), string=match.group().decode("ascii"), offset=match.start(), encoding=StringEncoding.ASCII, length=len(match.group().decode('ascii')))

[williballenthin]

I concur with this comment. Using the string offset as an identifier is probably a better solution, because it maps nicely to the identity of the string.

[williballenthin]

I will acknowledge that #721 does suggest using an id field, but I still believe that the string address/offset should be the id, rather than an incrementing integer.

[gemini-code-assist]

Using int(i+1) to assign IDs might be problematic if the extraction logic changes or if there are multiple calls to these functions. Consider using a dedicated counter or generator to ensure uniqueness.

            yield StaticString(
                id=int(i + 1), string=match.group().decode("utf-16"), offset=match.start(), encoding=StringEncoding.UTF16LE, length=len(match.group().decode('utf-16'))
            )