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

json-parser: basic support for ShellCheck's JSON output #83

Merged
merged 1 commit into from
Oct 3, 2022

Conversation

kdudka
Copy link
Member

@kdudka kdudka commented Sep 8, 2022

... which is produced by shellcheck --format=json1 ...

@kdudka
Copy link
Member Author

kdudka commented Sep 30, 2022

I think this is safe to be merged. We can extend the support for ShellCheck's JSON output in subsequent merge requests.

@kdudka kdudka marked this pull request as ready for review September 30, 2022 08:31
@kdudka kdudka requested a review from lzaoral September 30, 2022 08:31
@lzaoral lzaoral requested a review from jamacku September 30, 2022 08:33
Copy link
Member

@jamacku jamacku left a comment

Choose a reason for hiding this comment

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

@kdudka Thank you. Only one small thing. I think the commit message should say --format=json1 instead of only json as mentioned in:

kdudka added a commit to kdudka/csdiff that referenced this pull request Sep 30, 2022
... which is produced by `shellcheck --format=json1 ...`

Fixes: csutils#75
Closes: csutils#83
@kdudka
Copy link
Member Author

kdudka commented Sep 30, 2022

@jamacku Good catch! Fixed.

Copy link
Member

@lzaoral lzaoral left a comment

Choose a reason for hiding this comment

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

The code looks great but I'd drop the Fixes: from the commit message. The issue was specifically asking about handling of ShellCheck's suggestions which are still marked as a TODO.

Edit: Maybe Related: is more appropriate.

... which is produced by `shellcheck --format=json1 ...`

Related: csutils#75
Closes: csutils#83
@kdudka
Copy link
Member Author

kdudka commented Sep 30, 2022

@lzaoral You are right. Fixed.

Copy link
Member

@lzaoral lzaoral left a comment

Choose a reason for hiding this comment

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

Thanks, @kdudka!

@lzaoral lzaoral requested a review from jamacku September 30, 2022 15:05
@kdudka kdudka closed this in 3ab6fbf Oct 3, 2022
@kdudka kdudka merged commit 3ab6fbf into csutils:main Oct 3, 2022
@kdudka
Copy link
Member Author

kdudka commented Oct 3, 2022

@lzaoral Note that using Related: in the commit message does not prevent GitHub from closing the issue.

@lzaoral
Copy link
Member

lzaoral commented Oct 3, 2022

Sorry, @kdudka. I did not know that and it is a rather stupid behaviour.

@kdudka
Copy link
Member Author

kdudka commented Oct 5, 2022

My bad. The issue was explicitly linked with the pull request on GitHub. The use of Related: in the commit message was in fact unrelated to my observation. @lzaoral Thank you for pointing it out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please add support for ShellCheck JSON format
3 participants