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

Add ids to second occurrence of a type within a dump #60

Merged
merged 5 commits into from
Oct 3, 2022

Conversation

tahirmt
Copy link
Contributor

@tahirmt tahirmt commented Sep 24, 2022

Adding a unique id for occurrences of the same type.

Motivation

  • Whenever the same object is repeated within a dump it gets completely redacted. This is done to avoid recursion but it can also mean that if there is the same object the appears multiple times for other reasons it still gets redacted
  • If there is a repetition of multiple objects of the same type it removes the information about which object it is in the redacted state
  • By assigning an id to each second or more occurrence of the same type we can identify which object it is easily

@stephencelis
Copy link
Member

This is definitely an improvement! Brandon and I will look at this and maybe bikeshed some formatting details on Monday.

let id = idPerItem[item, default: occurence]
idPerItem[item] = id

return id > 1 ? "<\(id)>" : ""
Copy link
Member

Choose a reason for hiding this comment

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

One thought (suggested by Brandon) is to maybe use # instead of <> to avoid clashing with generic parameter list syntax:

Suggested change
return id > 1 ? "<\(id)>" : ""
return id > 1 ? "#\(id)" : ""

Thoughts? Any concerns with such a change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@stephencelis stephencelis left a comment

Choose a reason for hiding this comment

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

Looks good! Just one change we'd like to consider.

@tahirmt tahirmt force-pushed the repeated-items-unique-id branch from ad7129e to 457714a Compare October 3, 2022 17:01
@tahirmt tahirmt force-pushed the repeated-items-unique-id branch from 457714a to fb714f5 Compare October 3, 2022 19:40
@stephencelis stephencelis merged commit 8e64f1e into pointfreeco:main Oct 3, 2022
@tahirmt tahirmt deleted the repeated-items-unique-id branch October 3, 2022 20:44
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.

2 participants