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

Use Self #502

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

Use Self #502

wants to merge 2 commits into from

Conversation

jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Feb 8, 2024

Removes some clippy warnings about https://rust-lang.github.io/rust-clippy/master/index.html#/use_self

There are others. I'm working on them.

@jayvdb jayvdb marked this pull request as ready for review February 8, 2024 01:23
@ahl
Copy link
Collaborator

ahl commented Feb 8, 2024

I've come to the belief that the right thing to do for clippy on generated code is to disable it--the purpose of clippy is to make code more idiomatic, and while I think that's a good goal even for generated code, it is not worth the cost of bifurcation or complicating the code generator.

So the code in this commit looks good. The change is simple and doesn't make anything more complicated.

However, I think the code here add complexity without adding value.

If you disagree with this assessment I would be happy to hear your perspective, and thank you as always for the contribution.

@jayvdb
Copy link
Contributor Author

jayvdb commented Feb 19, 2024

Sorry for the delay @ahl. I do appreciate the added complexity is a concern. I especially hate the addition of a bool throughout typify-impl/src/value.rs which is the commit you dislike, and welcome any suggestions on what you'll accept to make that be neater / more literate. The obvious answer is an Options struct arg to provide call-by-keyword-named-args. Happy to use anything like that to make the calling convention more literate. The existing scope arg was also interesting but I didnt investigate it - perhaps that could used, as Self is a scope.

In our situation, we write the progenitor generated code to the src/ folder for each client. This is done because we are in a highly regulated industry, and need to carefully audit all of our dependencies, and separate them into SOUP, COTS, OTS. The easiest way to approach this with codegen dependencies is to be able to build without the codegen dependencies, removing them from the production build dep tree. Writing into src/ also ensures all generated changes are reviewed - handy when upstream modifies things a bit.

In addition our software development process documents state that we'll use clippy & fmt , and so legally we must. Altering these process docs to make it conditional invites problems. The result is we have devs implicitly running build.rs generating non-clippy compliant changes, and these changes often ending up in PRs, causing lots of wasted CI and annoyed devs as they need to revert those changes. Having typify/progenitor comply with the default clippy rules would avoid this. Again I appreciate this is a moving target, and which rules become default enabled is a bit arbitrary, but the benefit of adopting that approach is that there is a defined list, all tools used that list by default, and it is easy to sell to auditors as the appropriate/official set of lints that should be complied with.

@jayvdb
Copy link
Contributor Author

jayvdb commented Feb 19, 2024

So the code in this commit looks good.

feel free to cherry pick that one, or I can put that in a separate PR. I'm happy to take the wins I can get.

@ahl
Copy link
Collaborator

ahl commented Apr 3, 2024

With regard to your comment about requiring your code (generated or otherwise) to be clippy-clean: are attributes that disable clippy permitted?

@jayvdb
Copy link
Contributor Author

jayvdb commented Apr 3, 2024

Using #[allow(clippy::all)] is not. I'm going to have to remove that using syn post-processing. Specific clippy allows are ok, if placed directly on the code where the problem exists. i.e. the generated code needs to go through the same code review process as if written by a human. If we wouldnt let a human do it, we cant let a computer do it.

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