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 a script to manage marker traits #793

Closed
wants to merge 10 commits into from

Conversation

alexfertel
Copy link
Contributor

@alexfertel alexfertel commented Jul 4, 2021

Closes #756.

This PR introduces a Python script named public-types-tests.py that manages marker traits for the public API types. Following the discussion in #756, this script is responsible for two tasks:

  • Generate a test that asserts marker traits being implemented on the public API types (This test might need to be tweaked so it passes). This is done by generating the docs for this crate, parsing them, and
    writing the assertions to the file tests/marker_traits.rs. This shouldn't have to be done very often, probably just once. We pass the -g flag as an argument to the script to generate the tests.
  • Check the existence of marker-trait tests for each public API type, failing if it doesn't find at least one of the possible four.

This accomplishes three objectives:

  • Automate generating the assertion for each type.
  • Failing CI when a new type is added because no test related to it exists.
  • Avoid regressions in current types having marker traits auto-implemented.

Two things remain for this PR to be ready:

  • Add comments to the script.
  • Setup CI.

@alexfertel
Copy link
Contributor Author

@BurntSushi Let me know if this is somewhat near what you had in mind.

@alexfertel alexfertel marked this pull request as ready for review July 9, 2021 17:04
@alexfertel
Copy link
Contributor Author

@BurntSushi Sorry to bother, is something blocking this PR, or is it way too different from what you expected? 🥺

@BurntSushi
Copy link
Member

Haven't had a chance to look yet, sorry. I have a baby at home and the past few weeks have been hectic.

@alexfertel
Copy link
Contributor Author

@BurntSushi Oh, no worries at all. Congrats!!! Sorry for bothering 😅

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

OK, so I've finally had a chance to look at this. I also re-read #756, and I feel like there was a miscommunication perhaps. In particular, I specifically do not want extra automation for generating the tests. It's effectively extra code for almost no value. The public API just does not change much. So adding a new test by hand is done once in a blue moon and it doesn't cost much to do.

The only thing I really want here is something to tell me whether a new test needs to be added or not, and then run that in CI. I see that here, which is great. But I really think it should just be a shell script. I have almost gotten rid of Python from this repo, so I'd rather not bring more of it in.

I realize this is a really tardy review, so feel free to just drop this. The baby has kept me very busy. But if you're looking to keep going here, I'd say:

  1. Drop all the codegen.
  2. Convert the existence check to a simple shell script.

@BurntSushi
Copy link
Member

I'm going to close this out due to inactivity, but I would still like something simple that checks that we're covering the API with tests for marker traits.

@BurntSushi BurntSushi closed this Mar 4, 2023
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.

test marker traits more rigorously on public API types
2 participants