Skip to content

Conversation

@manumafe98
Copy link
Contributor

@manumafe98 manumafe98 commented Feb 25, 2024

The goal is to update the analyzer to how we are currently implementing them

Todo:

@manumafe98 manumafe98 added the x:size/small Small amount of work label Feb 25, 2024
@manumafe98 manumafe98 self-assigned this Feb 25, 2024
@manumafe98 manumafe98 requested a review from a team as a code owner February 25, 2024 00:37
@sanderploegsma
Copy link
Contributor

I would like to note the following though:

  1. I noticed you added a lot of extra smoke tests. This is nice, but since we have most of the approaches covered in unit tests already there is technically no need to add a smoke test for each approach as well. I try to aim for 2-3 smoke tests per exercise.
  2. I'd like to point out that each exercise analyzer is free to choose the best approach regarding implementation. The fact that most analyzers apply the visitor pattern doesn't mean that all of them have to. There may be exercises where walking the AST using the .walk method is more beneficial, or perhaps a new approach works well for some. As long as they satisfy the existing Analyzer interface and produce correct results, I don't think we should enforce a convention. To that end, I would like to suggest that we don't refactor the HammingAnalyzer, simply to show that there are more ways to implement new analyzers.

@manumafe98 manumafe98 added x:size/medium Medium amount of work and removed x:size/small Small amount of work labels Feb 29, 2024
@manumafe98 manumafe98 merged commit a810853 into exercism:main Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

x:size/medium Medium amount of work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants