Skip to content

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

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

Jump to bottom
Merged
kdudka merged 1 commit into from
Oct 3, 2022
Merged

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

kdudka merged 1 commit into from
Oct 3, 2022

Conversation

@kdudka
Copy link
Member

@kdudka kdudka commented Sep 8, 2022 •
edited by jamacku
Loading

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

@kdudka kdudka mentioned this pull request Sep 8, 2022
Open
@jamacku jamacku mentioned this pull request Sep 8, 2022
Closed
1 task
@jamacku jamacku linked an issue Sep 8, 2022 that may be closed by this pull request
Please add support for ShellCheck JSON format #75
Open
@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
jamacku
jamacku requested changes Sep 30, 2022
View reviewed changes
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
@kdudka
json-parser: basic support for ShellCheck's JSON output
5b7b712 
... 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.

lzaoral
lzaoral requested changes Sep 30, 2022
View reviewed changes
Copy link
Member

@lzaoral lzaoral left a comment •
edited
Loading

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.

@kdudka
json-parser: basic support for ShellCheck's JSON output
3ab6fbf 
... 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.

lzaoral
lzaoral approved these changes Sep 30, 2022
View reviewed changes
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 •
edited
Loading

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

Reviewers

@jamacku jamacku jamacku approved these changes

@lzaoral lzaoral lzaoral approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Please add support for ShellCheck JSON format

3 participants

@kdudka @lzaoral @jamacku