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

First pass at implementing 'trait_missing.ron' #115

Closed
wants to merge 3 commits into from

Conversation

seanchen1991
Copy link

Closes #73

I was referencing the struct_missing.ron file, and in there there's a struct_type @output @tag line in the query that doesn't exist in the enum lint. Curious what this is for.

TODOs

  • Add trait_missing.ron query file
  • Add trait_missing.rs file
  • Add test case(s)
  • Add the outputs you expect your query to produce over your test case in a new file: src/test_data/<query_name>.output.run.
  • Add <query_name> to the list of queries tested by the query_execution_tests!() macro near the bottom of src/adapter.rs.
  • Re-run ./scripts/regenerate_test_rustdocs.sh to generate the new rustdoc JSON file.
  • Run cargo test and ensure your new test appears in the test list and runs correctly.
  • Add an include_str!("queries/<query_name>.ron"), line to SemverQuery::all_queries() in the src/query.rs file, to ensure your query is enabled for use in query runs.

Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

The query looks good!

src/queries/trait_missing.ron Outdated Show resolved Hide resolved
@obi1kenobi
Copy link
Owner

obi1kenobi commented Aug 31, 2022

there's a struct_type @output @tag line in the query that doesn't exist in the enum lint. Curious what this is for.

This is because Rust has three different kinds of structs: unit structs Foo, tuple structs Bar(u32) and "regular" structs

struct Baz {
    x: u32,
    y: u32,
}

But you raise an interesting question — perhaps that @tag and @filter shouldn't be there after all. By trying to match a struct's kind between the baseline and current JSON, we'd report a struct as "missing / renamed / moved" if it merely changed from one kind to another. This isn't the most helpful outcome since the error message would be somewhat misleading. We'd be better off reporting that in a check specific to that error.

There's already a check that checks part of this (unit_struct_changed_kind), but we need to check more cases there, as this post describes: rust-lang/cargo#10871 (comment)

You're welcome to take on those checks as well, if you'd like. I added it in #5 as struct with public fields changes to another kind. I'm happy to mentor as well, if you'd like.

@obi1kenobi
Copy link
Owner

In #117 I added one line to unblock this PR, so you don't have to debug "why are traits never found as items" and then edit the data bindings (Trustfall impl Adapter) code. If you're interested in continuing to contribute after this PR, I'm happy to guide you into that code as well — there's no shortage of opportunities here.

@obi1kenobi
Copy link
Owner

No rush at all, just checking in to see how you're doing. Any questions? Anything I can do to help?

@seanchen1991
Copy link
Author

Hi, I'm traveling out of the country currently, so I won't probably won't get around to working on this until the week of the 19th.

1 similar comment
@seanchen1991
Copy link
Author

Hi, I'm traveling out of the country currently, so I won't probably won't get around to working on this until the week of the 19th.

@obi1kenobi
Copy link
Owner

Hi, I'm traveling out of the country currently, so I won't probably won't get around to working on this until the week of the 19th.

No worries at all, no rush. Enjoy your travel! I just wanted to make sure you aren't blocked on me.

@obi1kenobi
Copy link
Owner

Hi! We recently merged a significant revamp of the directory structure and how the tests are organized, and if you're interested in continuing with this PR either now or in January, I wanted to let you know that I'd be happy to help.

If you've moved on to other projects, that's totally fine — no worries! Just let me know so I can reassign this lint to someone else.

If I don't hear back by Jan 15, I'll assume you've moved on to other things and I'll close this PR.

@seanchen1991
Copy link
Author

Hi, thanks for following up on this. Yes, you can assign this to someone else. Sorry for the long radio silence.

@obi1kenobi
Copy link
Owner

No worries, thanks for the heads-up!

@obi1kenobi obi1kenobi closed this Dec 14, 2022
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.

pub trait moved, deleted, or renamed
2 participants