Skip to content

Revert "[Bug] Handle formatting empty list" #4087

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

brokensound77
Copy link
Contributor

Reverts #4086

This technically introduced a bug and will create opaque issues - most specifically when dumping a rule with empty lists then reparsing them (if required by the schema)

details:

#4086 (comment)

Copy link
Contributor

@Mikaayenson Mikaayenson left a comment

Choose a reason for hiding this comment

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

Id either make the changes suggested here or open a PR that will. Also if you have an example of this, can you share?

@Mikaayenson Mikaayenson removed their assignment Sep 18, 2024
@brokensound77
Copy link
Contributor Author

Id either make the changes suggested here or open a PR that will. Also if you have an example of this, can you share?

huh?

This isn't for resolving your original issue, it is to remove the faulty code that was introduced. Re-implementing a solution the proper way should occur in a dedicated PR, not alongside changes within a reverted commit

@Mikaayenson
Copy link
Contributor

Id either make the changes suggested here or open a PR that will. Also if you have an example of this, can you share?

huh?

This isn't for resolving your original issue, it is to remove the faulty code that was introduced. Re-implementing a solution the proper way should occur in a dedicated PR, not alongside changes within a reverted commit

Do you have an example error or stack trace you can share? Steps to reproduce the error would be most helpful.

@botelastic
Copy link

botelastic bot commented Nov 18, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@botelastic botelastic bot added the stale 60 days of inactivity label Nov 18, 2024
@botelastic
Copy link

botelastic bot commented Nov 25, 2024

This has been closed due to inactivity. If you feel this is an error, please re-open and include a justifying comment.

@botelastic botelastic bot closed this Nov 25, 2024
@brokensound77 brokensound77 reopened this Dec 3, 2024
@botelastic botelastic bot removed the stale 60 days of inactivity label Dec 3, 2024
@botelastic
Copy link

botelastic bot commented Feb 1, 2025

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@botelastic botelastic bot added the stale 60 days of inactivity label Feb 1, 2025
@@ -161,7 +161,7 @@ def dump_list(self, v):
dump.append(' ' * 4 + self.dump_value(item))
return '[\n{},\n]'.format(',\n'.join(dump))

if v and all(isinstance(i, dict) for i in v):
if all(isinstance(i, dict) for i in v):
Copy link
Contributor

@traut traut Feb 3, 2025

Choose a reason for hiding this comment

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

I think we can make it clearer! Reading the code, it's not obvious that this check will return True for an empty list / iterator, but it will:

In [1]: all([])
Out[1]: True

In [2]: all(x for x in [])
Out[2]: True

If that's desirable, we can make it explicit by doing

if len(v) == 0 or all(isinstance(i, dict) for i in v):

@botelastic botelastic bot removed the stale 60 days of inactivity label Feb 3, 2025
@Mikaayenson Mikaayenson added the wontfix This will not be worked on label Feb 11, 2025
@botelastic
Copy link

botelastic bot commented May 14, 2025

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@botelastic botelastic bot added the stale 60 days of inactivity label May 14, 2025
@brokensound77 brokensound77 added the bug Something isn't working label May 16, 2025
@botelastic botelastic bot removed the stale 60 days of inactivity label May 16, 2025
Copy link
Contributor

Bug - Guidelines

These guidelines serve as a reminder set of considerations when addressing a bug in the code.

Documentation and Context

  • Provide detailed documentation (description, screenshots, reproducing the bug, etc.) of the bug if not already documented in an issue.
  • Include additional context or details about the problem.
  • Ensure the fix includes necessary updates to the release documentation and versioning.

Code Standards and Practices

  • Code follows established design patterns within the repo and avoids duplication.
  • Code changes do not introduce new warnings or errors.
  • Variables and functions are well-named and descriptive.
  • Any unnecessary / commented-out code is removed.
  • Ensure that the code is modular and reusable where applicable.
  • Check for proper exception handling and messaging.

Testing

  • New unit tests have been added to cover the bug fix or edge cases.
  • Existing unit tests have been updated to reflect the changes.
  • Provide evidence of testing and detecting the bug fix (e.g., test logs, screenshots).
  • Validate that any rules affected by the bug are correctly updated.
  • Ensure that performance is not negatively impacted by the changes.
  • Verify that any release artifacts are properly generated and tested.

Additional Checks

  • Ensure that the bug fix does not break existing functionality.
  • Review the bug fix with a peer or team member for additional insights.
  • Verify that the bug fix works across all relevant environments (e.g., different OS versions).
  • Confirm that all dependencies are up-to-date and compatible with the changes.
  • Confirm that the proper version label is applied to the PR patch, minor, major.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport: auto bug Something isn't working python Internal python for the repository wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants