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

Remove unneeded Debug trait requirement from BasicSnippet and DeprecatedSnippet #82

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Sword-Smith
Copy link
Contributor

No description provided.

@Sword-Smith Sword-Smith force-pushed the remove-debug-requirement-from-basicsnippet branch from a88be68 to 7486a0b Compare February 23, 2024 13:59
@Sword-Smith Sword-Smith changed the title Remove unneeded Debug trait requirement from BasicSnippet Remove unneeded Debug trait requirement from BasicSnippet and DeprecatedSnippet Feb 23, 2024
@jan-ferdinand
Copy link
Member

Does this trait requirement interfere with anything? Do the #[derive(Debug)] interfere with anything? It is common practice to #[derive(Debug)] wherever possible, unless there's a good reason not to do so, of which I can find few and none that seem to apply here.

@Sword-Smith Sword-Smith force-pushed the remove-debug-requirement-from-basicsnippet branch from 7486a0b to 7fe2499 Compare February 23, 2024 14:01
@Sword-Smith
Copy link
Contributor Author

Does this trait requirement interfere with anything? Do the #[derive(Debug)] interfere with anything? It is common practice to #[derive(Debug)] wherever possible, unless there's a good reason not to do so, of which I can find few and none that seem to apply here.

I see two arguments for removing Debug

  1. It puts a Debug trait constraint on fields in snippet structs that implement BasicSnippet
  2. It's not needed anywhere as a snippet should be fully identified by its entrypoint. So adding it gives the compiler more work than necessary.

@jan-ferdinand
Copy link
Member

  1. I return to my first question; does the trait bound interfere with anything?
  2. I'd be very surprised if that additional workload was significant.

In the end, this is your codebase; do as you see fit.

@Sword-Smith
Copy link
Contributor Author

  1. I return to my first question; does the trait bound interfere with anything?

    1. I'd be very surprised if that additional workload was significant.

In the end, this is your codebase; do as you see fit.

  1. Not currently. I could, theoretically, in the future.

@Sword-Smith
Copy link
Contributor Author

I guess my opinion is more based on aesthetics and how I think you should inspect values of this type. Aesthetically I don't like to impose trait bounds that are not needed, and I think that you should inspect the the type of a snippet through its entrypoint name, not through its debug output.

@jan-ferdinand
Copy link
Member

If that is the case, then how about the following?

impl Debug for dyn BasicSnippet {
    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
        write!(f, "{}", self.entrypoint())
    }
}

This would also allow the #[derive(Debug)] on the various types to stay in place.

@Sword-Smith
Copy link
Contributor Author

Your suggestion works. But I should also like to point out that no Debug derivations are removed with this PR, it's only the requirement that is removed.

@jan-ferdinand
Copy link
Member

jan-ferdinand commented Feb 23, 2024

What about removal of line 35 in all.rs, line 31 in filter.rs, line 83 in inner_function.rs, and line 37 of map.rs?

@Sword-Smith
Copy link
Contributor Author

What about removal of line 35 in all.rs, line 31 in filter.rs, line 83 in inner_function.rs, and line 37 of map.rs?

You're right. It got removed from higher-order snippets.

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