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.

Sign up for GitHub

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

Proposal for tagging mechanism #79

Merged
merged 18 commits into from
Dec 13, 2023
Merged

Proposal for tagging mechanism #79

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
5cc72c3
Add destructuring helper
jiegillet Nov 23, 2023
395f0dd
generate tags
jiegillet Nov 23, 2023
9daf799
compare sorted expected values
jiegillet Nov 23, 2023
8f6c54e
remove unused things
jiegillet Nov 23, 2023
5ddf8f0
Merge branch 'main' into jie-tags
jiegillet Nov 25, 2023
aa906dc
ProjectContext become record type
jiegillet Nov 25, 2023
bd4af55
Add smoke test mechanism
jiegillet Nov 25, 2023
6a64b85
Bug fix: don't overwrite extracts
jiegillet Nov 25, 2023
b9afae7
tags ignore files that errors ignore
jiegillet Nov 25, 2023
ad9738c
Remove Tag alias
jiegillet Nov 25, 2023
2d620b1
Separate type expressions
jiegillet Nov 25, 2023
beea31b
Adding some tagd
jiegillet Nov 25, 2023
ca1a013
tagging comments
jiegillet Nov 25, 2023
2be7eec
type declaration tags
jiegillet Nov 25, 2023
2c21bcf
destructuring
jiegillet Nov 26, 2023
7efeab8
smoke tests pass
jiegillet Dec 9, 2023
27dffad
Add feature flag
jiegillet Dec 9, 2023
335aa1b
fix elm-review errors
jiegillet Dec 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@ elm-stuff
bin/*.js
src/main.js
*analysis.json
*tags.json
node_modules
4 changes: 2 additions & 2 deletions bin/check_files.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ function main {
exit 1
fi

jq -S . ${exercise}/expected_analysis.json > /tmp/expected.json
jq -S . ${exercise}/analysis.json > /tmp/actual.json
jq -S '.comments |= sort' ${exercise}/expected_analysis.json > /tmp/expected.json
jq -S '.comments |= sort' ${exercise}/analysis.json > /tmp/actual.json
if ! diff /tmp/expected.json /tmp/actual.json ;then
echo "🔥 ${exercise}: expected ${exercise}/analysis.json to equal ${exercise}/expected_analysis.json on successful run 🔥"
exit 1
Expand Down
13 changes: 11 additions & 2 deletions bin/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,23 @@ if [ -f solution_cache.tar ]; then
INPUT_DIR=/tmp/sol
fi

# Run analysis
# Temporarily disable -e mode
set +e

# Run analysis
npx elm-review $INPUT_DIR \
--elmjson $INPUT_DIR/elm.json \
--config . \
--report=json \
| node ./bin/cli.js > $OUTPUT_DIR/analysis.json
--extract \
> /tmp/elm-review-report.json

# Get comments
cat /tmp/elm-review-report.json | node ./bin/cli.js > $OUTPUT_DIR/analysis.json

# Get tags
jq '.extracts .Tags' /tmp/elm-review-report.json > $OUTPUT_DIR/tags.json

set -e

echo Finished
27 changes: 26 additions & 1 deletion src/ElmSyntaxHelpers.elm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
module ElmSyntaxHelpers exposing (hasGenericRecord, traversePattern, typeAnnotationsMatch)
module ElmSyntaxHelpers exposing (hasDestructuringPattern, hasGenericRecord, traversePattern, typeAnnotationsMatch)

import Elm.Syntax.Node as Node exposing (Node(..))
import Elm.Syntax.Pattern exposing (Pattern(..))
Expand Down Expand Up @@ -76,6 +76,31 @@ hasGenericRecord annotation =
False


hasDestructuringPattern : Node Pattern -> Bool
hasDestructuringPattern fullPattern =
let
destructuringPattern pattern =
case Node.value pattern of
RecordPattern _ ->
True

UnConsPattern _ _ ->
True

TuplePattern _ ->
True

NamedPattern _ _ ->
True

_ ->
False
in
fullPattern
|> traversePattern
|> List.any destructuringPattern


traversePattern : Node Pattern -> List (Node Pattern)
traversePattern pattern =
case Node.value pattern of
Expand Down
8 changes: 6 additions & 2 deletions src/ReviewConfig.elm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,16 @@ import Exercise.ValentinesDay
import Exercise.ZebraPuzzle
import Review.Rule as Rule exposing (Rule)
import RuleConfig exposing (RuleConfig)
import Tags


ruleConfigs : List RuleConfig
ruleConfigs =
[ -- Common Rules
Common.NoUnused.ruleConfig
[ -- Tags
Tags.ruleConfig

-- Common Rules
, Common.NoUnused.ruleConfig
, Common.Simplify.ruleConfig
, Common.NoDebug.ruleConfig
, Common.UseCamelCase.ruleConfig
Expand Down
18 changes: 14 additions & 4 deletions src/RuleConfig.elm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ type alias RuleConfig =
type AnalyzerRule
= CustomRule (Comment -> Rule) Comment
| ImportedRule Rule (Comment -> Decoder Comment) Comment
| TagRule Rule


analyzerRuleToRule : AnalyzerRule -> Rule
Expand All @@ -25,6 +26,9 @@ analyzerRuleToRule analyzerRule =
ImportedRule rule _ _ ->
rule

TagRule rule ->
rule


analyzerRuleToDecoder : AnalyzerRule -> Maybe (Decoder Comment)
analyzerRuleToDecoder analyzerRule =
Expand All @@ -35,15 +39,21 @@ analyzerRuleToDecoder analyzerRule =
ImportedRule _ toDecoder comment ->
Just (toDecoder comment)

TagRule _ ->
Nothing


analyzerRuleToComment : AnalyzerRule -> Comment
analyzerRuleToComment : AnalyzerRule -> Maybe Comment
analyzerRuleToComment analyzerRule =
case analyzerRule of
CustomRule _ comment ->
comment
Just comment

ImportedRule _ _ comment ->
comment
Just comment

TagRule _ ->
Nothing


getRules : RuleConfig -> List Rule
Expand All @@ -63,7 +73,7 @@ getDecoders =

getComments : List RuleConfig -> List Comment
getComments =
List.concatMap (.rules >> List.map analyzerRuleToComment)
List.concatMap (.rules >> List.filterMap analyzerRuleToComment)


makeConfig : List RuleConfig -> List Rule
Expand Down
217 changes: 217 additions & 0 deletions src/Tags.elm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,217 @@
module Tags exposing (commonTagsRule, expressionTagsRule, ruleConfig)

import Elm.Syntax.Expression exposing (Expression(..), LetDeclaration(..))
import Elm.Syntax.Node as Node exposing (Node(..))
import ElmSyntaxHelpers
import Json.Encode as Encode exposing (Value)
import Review.ModuleNameLookupTable as LookupTable exposing (ModuleNameLookupTable)
import Review.Rule as Rule exposing (Rule)
import RuleConfig exposing (AnalyzerRule(..), RuleConfig)
import Set exposing (Set)


type alias Tag =
String


type alias ProjectContext =
Set Tag


type alias ModuleContext =
{ lookupTable : ModuleNameLookupTable
, tags : Set Tag
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be ProjectContext?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmh that's an interesting question. I would say no, because conceptually I'm not trying to hold the ProjectContext inside of the ModuleContext, the ProjectContext happens to only have tags in it, but it could potentially need more things.
I think a satisfying solution would be to define type alias ProjectContext = { tags: Set Tag }.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't want to derail the conv so I said nothing before, but now that's on the table ... I tend to forbid all type alias that are trivial renames in my code bases. I always prefer having something like { context : Set Tag } instead of type alias Context = Set Tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would you do for type alias Tag = String?
It really does make the code easier to read for me, and wrapping would be quite noisy. Would you wrap it anyway? Or give up and use String?

Copy link
Member

Choose a reason for hiding this comment

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

-- I'd change the following
type alias Tag = String
type alias ProjectContext = Set Tag
type alias ModuleContext = { ..., tags : Set Tag }
commonTags : Set Tag
fromProjectToModule : Rule.ContextCreator ProjectContext ModuleContext
fromModuleToProject : Rule.ContextCreator ModuleContext ProjectContext
dataExtractor : ProjectContext -> Value
matchExpression : Node Expression -> Set Tag

-- Into the following (no Tag alias, no ProjectContext alias)
type alias ModuleContext = { ..., tags : Set String }
commonTags : Set String
fromProjectToModule : Rule.ContextCreator { tags : Set String } ModuleContext
fromModuleToProject : Rule.ContextCreator ModuleContext { tags : Set String }
dataExtractor : { tags : Set String } -> Value
-- also changed the name here
tagMatchExpression : Node Expression -> Set String

}


ruleConfig : RuleConfig
ruleConfig =
{ restrictToFiles = Nothing
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this include any template type files that are provided along with the solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it will, that's a good point, the data extractor will run on the whole project (it can even look at the elm.json if we want).
Do we want to skip the support files? If we do, we should enforce a convention on their names (I think we already do that, but it should be in the CI).

@ErikSchierboom what does the C# tagger do when it comes to solutions with editor files in it? Skip it? Include it?

You just made me realize that this will probably tag the tests as well, which I guess we don't want, so I'll try to avoid that.

Adding file names in the field restrictToFiles won't work though, because it's eventually fed Rule.filterErrorsForFiles which I think only works for errors, not for data extractions. I should rename it to reflect that.

Copy link
Member

Choose a reason for hiding this comment

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

@ErikSchierboom what does the C# tagger do when it comes to solutions with editor files in it? Skip it? Include it?

I skip editor files (and example, exemplar, test and invalidator files)

, rules =
[ TagRule commonTagsRule
, TagRule expressionTagsRule
]
}


commonTagsRule : Rule
commonTagsRule =
Rule.newProjectRuleSchema "Tags" commonTags
|> Rule.withModuleVisitor (Rule.withSimpleModuleDefinitionVisitor (always []))
|> Rule.withModuleContextUsingContextCreator
{ fromModuleToProject = fromModuleToProject
, fromProjectToModule = fromProjectToModule
, foldProjectContexts = Set.union
}
|> Rule.withDataExtractor dataExtractor
|> Rule.fromProjectRuleSchema


commonTags : Set Tag
commonTags =
Set.fromList
[ "paradigm:functional"
, "technique:immutability"
, "uses:module"
]


fromProjectToModule : Rule.ContextCreator ProjectContext ModuleContext
fromProjectToModule =
Rule.initContextCreator
(\lookupTable _ ->
{ lookupTable = lookupTable
, tags = Set.empty
}
)
|> Rule.withModuleNameLookupTable


fromModuleToProject : Rule.ContextCreator ModuleContext ProjectContext
fromModuleToProject =
Rule.initContextCreator (\{ tags } -> tags)


dataExtractor : ProjectContext -> Value
dataExtractor =
Set.toList >> Encode.list Encode.string


expressionTagsRule : Rule
expressionTagsRule =
Rule.newProjectRuleSchema "Tags" Set.empty
|> Rule.withModuleVisitor (Rule.withExpressionEnterVisitor expressionVisitor)
|> Rule.withModuleContextUsingContextCreator
{ fromModuleToProject = fromModuleToProject
, fromProjectToModule = fromProjectToModule
, foldProjectContexts = Set.union
}
|> Rule.withDataExtractor dataExtractor
|> Rule.fromProjectRuleSchema


expressionVisitor : Node Expression -> ModuleContext -> ( List never, ModuleContext )
expressionVisitor ((Node range expression) as node) ({ lookupTable, tags } as context) =
case expression of
FunctionOrValue _ name ->
case LookupTable.moduleNameFor lookupTable node of
Nothing ->
( [], { context | tags = Set.union tags (matchExpression node) } )

Just originalModuleName ->
( [], { context | tags = Set.union tags (matchExpression (Node range (FunctionOrValue originalModuleName name))) } )

_ ->
( [], { context | tags = Set.union tags (matchExpression node) } )


matchExpression : Node Expression -> Set Tag
matchExpression (Node range expression) =
case expression of
FunctionOrValue [ "Set" ] _ ->
Set.fromList [ "construct:set", "technique:immutable-collection", "technique:sorted-collection" ]

FunctionOrValue [ "Dict" ] _ ->
Set.fromList [ "construct:dictionary", "technique:immutable-collection", "technique:sorted-collection" ]

FunctionOrValue [ "Random" ] _ ->
Set.singleton "technique:randomness"

FunctionOrValue [ "Regex" ] _ ->
Set.singleton "technique:regular-expression"

FunctionOrValue [ "Debug" ] _ ->
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we forbid this in the Analyser elsewhere, as part of the common rules? Might be worth having here just in case though.

Copy link
Contributor Author

@jiegillet jiegillet Nov 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That rule is Actionable, so I think it's not blocking students from publishing a solution with a debug anyway. Also solutions published before the rule would have them.

BTW, that just made me think of adding a Debug concept, that might be worth doing.

Set.singleton "uses:debug"

PrefixOperator "+" ->
Set.singleton "construct:add"

OperatorApplication "+" _ _ _ ->
Set.singleton "construct:add"

PrefixOperator "-" ->
Set.singleton "construct:subtract"

OperatorApplication "-" _ _ _ ->
Set.singleton "construct:subtract"

PrefixOperator "*" ->
Set.singleton "construct:multiply"

OperatorApplication "*" _ _ _ ->
Set.singleton "construct:multiply"

PrefixOperator "/" ->
Set.singleton "construct:divide"

OperatorApplication "/" _ _ _ ->
Set.singleton "construct:divide"

PrefixOperator "//" ->
Set.singleton "construct:divide"

OperatorApplication "//" _ _ _ ->
Set.singleton "construct:divide"

PrefixOperator "==" ->
Set.fromList [ "construct:equality", "technique:equality-comparison", "construct:boolean" ]

OperatorApplication "==" _ _ _ ->
Set.fromList [ "construct:equality", "technique:equality-comparison", "construct:boolean" ]

PrefixOperator "/=" ->
Set.fromList [ "construct:inequality", "technique:equality-comparison", "construct:boolean" ]

OperatorApplication "/=" _ _ _ ->
Set.fromList [ "construct:inequality", "technique:equality-comparison", "construct:boolean" ]

UnitExpr ->
Set.singleton "uses:unit"

Floatable _ ->
Set.fromList [ "construct:float", "construct:floating-point-number" ]

Integer _ ->
Set.fromList [ "construct:integral-number", "construct:int" ]

Hex _ ->
Set.fromList [ "construct:hexadecimal-number", "construct:integral-number", "construct:int" ]

Negation _ ->
Set.singleton "construct:unary-minus"

Literal _ ->
if range.end.row > range.start.row then
Set.fromList [ "construct:string", "construct:multiline-string" ]

else
Set.singleton "construct:string"

LambdaExpression _ ->
Set.singleton "construct:lambda"

IfBlock _ _ _ ->
Set.fromList [ "construct:if", "construct:boolean" ]

LetExpression { declarations } ->
if List.any (Node.value >> usesProperDestructuring) declarations then
Set.fromList [ "uses:destructure", "construct:pattern-matching" ]
jiegillet marked this conversation as resolved.
Show resolved Hide resolved

else
Set.empty

_ ->
Set.empty


usesProperDestructuring : LetDeclaration -> Bool
usesProperDestructuring letDeclaration =
case letDeclaration of
LetDestructuring _ _ ->
True

LetFunction { declaration } ->
declaration
|> Node.value
|> .arguments
|> List.any ElmSyntaxHelpers.hasDestructuringPattern
Loading
Loading