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

Move linting code to under Stanc, tests #117

Merged
merged 5 commits into from
Jul 1, 2024
Merged

Move linting code to under Stanc, tests #117

merged 5 commits into from
Jul 1, 2024

Conversation

WardBrian
Copy link
Collaborator

Closes #105

Copy link
Collaborator

@jsoules jsoules left a comment

Choose a reason for hiding this comment

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

I'm fine with this as is, but spent way too much time making fussy little recommendations.

Thanks for including the tests; they're great for clarifying things. Looking at the coverage reports, you may want to include a test for both warnings and errors from included files.

You may also wish to write a test for stancErrorsToCodeMarkers, it's pretty trivial but I noticed it was currently uncovered, and it would probably cover stancMessageToMarker by incorporation. I suppose the behaviors to confirm would be that there are no empty elements in the list, that the errors are all mapped before the warnings, and that the messages track with the severity or something?

gui/src/app/Stanc/Linting.ts Outdated Show resolved Hide resolved
gui/src/app/Stanc/Linting.ts Outdated Show resolved Hide resolved
gui/src/app/Stanc/Linting.ts Outdated Show resolved Hide resolved
gui/src/app/Stanc/Linting.ts Outdated Show resolved Hide resolved
gui/src/app/Stanc/Linting.ts Show resolved Hide resolved
gui/test/app/Stanc/Linting.test.ts Outdated Show resolved Hide resolved
gui/src/app/Stanc/Linting.ts Outdated Show resolved Hide resolved
gui/test/app/Stanc/Linting.test.ts Outdated Show resolved Hide resolved
gui/test/app/Stanc/Linting.test.ts Show resolved Hide resolved
gui/test/app/Stanc/Linting.test.ts Show resolved Hide resolved
@WardBrian
Copy link
Collaborator Author

I added the tests you requested, expect for included files -- since we don't currently support that, I just removed the code. A bunch of stuff would need to change first anyway ;)

@WardBrian WardBrian requested a review from jsoules July 1, 2024 19:00
@WardBrian WardBrian merged commit 4d983cf into main Jul 1, 2024
2 checks passed
@WardBrian WardBrian deleted the linting-tests branch July 1, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Encapsulate string/error-parsing logic from StanFileEditor.tsx
2 participants