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

Implement analyzer for secrets #141

Merged
merged 7 commits into from
Mar 11, 2024

Conversation

manumafe98
Copy link
Contributor

@manumafe98 manumafe98 commented Mar 5, 2024

closes #112

Todo:

@manumafe98 manumafe98 added the x:size/medium Medium amount of work label Mar 5, 2024
@manumafe98 manumafe98 self-assigned this Mar 5, 2024
@manumafe98 manumafe98 requested a review from a team as a code owner March 5, 2024 00:09
@manumafe98 manumafe98 changed the title Implement analyzer for secrets Implement analyzer for secrets Mar 5, 2024
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Approving via hand waving :) Someone else should do the actual Java code review :)

@sanderploegsma
Copy link
Contributor

I found another case that we may be able to comment on:

public class Secrets {
    public static int shiftBack(int value, int amount) {
        if (value<0) return value>>>amount;
        return value>>amount;
    }
}

Perhaps we can add an actionable comment on this, saying that using >>> works for both positive and negative values and no if-statement is required.

@manumafe98
Copy link
Contributor Author

manumafe98 commented Mar 5, 2024

I found another case that we may be able to comment on:

public class Secrets {
    public static int shiftBack(int value, int amount) {
        if (value<0) return value>>>amount;
        return value>>amount;
    }
}

Perhaps we can add an actionable comment on this, saying that using >>> works for both positive and negative values and no if-statement is required.

Instead of being that specific I think I'm going to make a comment to avoid any conditional logic, so they focus only on using the correct operator

Udating Analyzer root to run the analyzer on alphabetical order
Changing implement operator comment to use bitwise operator
Updating the analyzer to only return 1 essential comment at a time
Generalizing the method that checks the usage of the operator
Adding extra comment to check for conditional logic
Updating preferBitwiseNot to trigger if the student used bitwise and
@manumafe98
Copy link
Contributor Author

@sanderploegsma I should add extra smoke tests? or we should limit to create 2/3?

@sanderploegsma
Copy link
Contributor

sanderploegsma commented Mar 7, 2024

@sanderploegsma I should add extra smoke tests? or we should limit to create 2/3?

The current amount is fine. The primary goal of these tests is to test the analyzer end-to-end just to make sure we didn't make a mistake that would otherwise be missed because it's not covered by the JUnit tests.

Basically we want to see that the analyzer provides some output for each exercise. It's not needed to cover all scenarios this way, that's what the JUnit tests are for.

@manumafe98 manumafe98 requested review from sanderploegsma and removed request for sanderploegsma March 11, 2024 14:41
Copy link
Contributor

@sanderploegsma sanderploegsma left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@manumafe98 manumafe98 merged commit 136383d into exercism:main Mar 11, 2024
4 checks passed
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.

secrets: implement analyzer
3 participants