Skip to content

Conversation

@Peter554
Copy link
Collaborator

@Peter554 Peter554 commented Jun 19, 2025

I've noticed some uses of module expressions in import linter contacts like this:

- domain 
- domain.**

We currently need two entries here, since domain.** does not include domain itself.

This observation motivates the change in this PR, which introduces a syntax for optional fragments within module expressions.

After this PR the above could be reduced to:

- domain[.**]

We didn't really discuss this before @seddonym so feel free to reject this or suggest alternatives!

I wanted to try out a agentic AI, which is the main personal reason for this PR. This PR was generated almost entirely by Claude 3.7 using Zed agentic AI.

Interestingly a module expression domain[.**] now behaves the same as domain with as_package=True.

@codspeed-hq
Copy link

codspeed-hq bot commented Jun 19, 2025

CodSpeed Instrumentation Performance Report

Merging #228 will not alter performance

Comparing Peter554:module-expression-optional-fragment (3bdcec8) with master (8a413e4)

Summary

✅ 22 untouched benchmarks

@Peter554 Peter554 marked this pull request as ready for review June 19, 2025 06:59
@Peter554 Peter554 force-pushed the module-expression-optional-fragment branch from 06498b6 to 3bdcec8 Compare June 19, 2025 08:59
return Err(GrimpError::InvalidModuleExpression(expression.to_owned()));
let mut patterns = Vec::new();
for expanded in &expanded_expressions {
// Validate each expanded expression
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea here is that after expanding the module expression, all of the expanded expressions should be valid by the old rules. E.g. a.b[.**] expands to a.b and a.b.**, both of which are valid expressions.

@seddonym
Copy link
Collaborator

Interesting to see how AI performs, but I'm not sure about the change itself. If someone is doing this in, say, a forbidden contract, can't they just remove as_packages = False?

@Peter554
Copy link
Collaborator Author

If someone is doing this in, say, a forbidden contract, can't they just remove as_packages = False?

@seddonym Yes they could - but as_packages = False is not available everywhere. E.g. in ignore_imports. Does that influence your opinion?

@seddonym
Copy link
Collaborator

I see what you mean. I think of ignore_imports being appropriate for specific exceptions (treated as tech debt burndown) rather than general allow-lists, but I appreciate that's not how everyone uses the tool.

I'm inclined to say that if someone is needing to use ignore_imports for a different purpose, it's a sign that we need a different contract type (in fact a 'protected' contract type is already in the pipeline). I'm also wary of making the expression matching syntax too powerful.

Let's leave this PR open for now - there are a few new contracts in the offing which may make it unnecessary, and plenty of other things that I'd like to concentrate on - in particular speeding up the linter.

Thanks for the idea though.

@Peter554
Copy link
Collaborator Author

Let's leave this PR open for now - there are a few new contracts in the offing which may make it unnecessary, and plenty of other things that I'd like to concentrate on - in particular speeding up the linter.

Okay sounds good to me - I'll put the PR back to draft for now then and we can revisit this if we see a need for it.

@Peter554 Peter554 marked this pull request as draft June 19, 2025 15:22
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