Closed m-kostrzewa closed 6 years ago
Perhaps add a unit test in syntax.rs to make sure that we're actually using case-insensitive scope-checking? Or perhaps we can say this is covered by integration test you've added (the unknown-functions lint doesn't fire).
:+1:
Integrate the check in scope::analyze. That would require adding &str (original string found in the set (you can use get)) to the Found enum. This might seem uglier (less separated) than previous solution with just passing &scopes, but it will be easier to integrate with improving scope analysis, which I'm going to implement, so I think I'll prefer this way.
:+1:
Protip: Annotate the PR with Fixes #
to automagically close the issues on merge!
:+1:
@krdln could you take a look?
Usage of bools (except of simple variables) is an antipattern in Rust and this type is not even creating a newtype, but just a type alias. Using enums is preferred.
Also, you return (Option
, IsValidCasing). What does the casing mean, when the item is not found? stuck_out_tongue I think something like that would be nice:
This was my original approach, but it takes like 4 lines (times two) to do it this way, when it takes one line with "bool" way. If the duplication I mentioned wasn't there, it wouldn't be an issue...
This would create a set of all defined and directly imported functions on every search! We certainly don't want to do that.
I'm just not that concerned about performance at this point :) but :+1:
@krdln so, lgty?
@m-kostrzewa What do you think about https://github.com/m-kostrzewa/shelly/pull/2 or https://github.com/m-kostrzewa/shelly/pull/3?
@krdln done
Fixes #11