githubtraining / looking-glass-action

provides feedback to learners based on the outcome of their validation lab grader workflow
MIT License
35 stars 10 forks source link

Readme updates #9

Closed hectorsector closed 3 years ago

hectorsector commented 3 years ago

Couple of quick fixes to the README.

hectorsector commented 3 years ago

[...] a payload signature. Looking Glass reads the output of a previous GitHub action. To use [...]

When I read this sentence, I'm expecting to also learn about what Looking Glass does with the output of the action. Maybe we can add something about reporting using the specified GitHub feature?

filename: "can be empty if no file is associated with the report",

In the second example, could we maybe just leave the filename off? We can include the "can be empty..." copy in the first example.

msg:
       "# something was incorrect\n**expected:** some correct answer\n**got:** some incorrect answewr",
   },

This is actually a pretty common pattern, message/expected/got. Should we support this?

mattdavis0351 commented 3 years ago

When I read this sentence, I'm expecting to also learn about what Looking Glass does with the output of the action. Maybe we can add something about reporting using the specified GitHub feature?

Sure, the details weren't my initial mindspace when writing this up, but if you have an idea let's add it and see!

In the second example, could we maybe just leave the filename off? We can include the "can be empty..." copy in the first example.

We can just leave it blank since it's shown as not required in the table of payload values.

This is actually a pretty common pattern, message/expected/got. Should we support this?

I like this idea, what if msg was

msg:{
    some_key: "general message to use",
    expected: "something",
    got: "this thing"
}

but we figure out a good name for some_key 🤣

mattdavis0351 commented 3 years ago

I'm also not sold on this payload being an array. I don't think Looking Glass should support more than one report for initial release.

We could keep it at an array an only focus on the first item in the array, that way we can extend this easier in the future, but we may want to add some sort of key to the payload that informs Looking Glass what kind of report it's getting

hectorsector commented 3 years ago

How about the following, ideally if we could support just msg: "some message" AND the longform:

msg:{
    context: "general message to use",
    expected: "something",
    got: "this thing"
}

I'm also not sold on this payload being an array.

I agree, but this is only relevant to the action itself right? What are the alternatives?

we may want to add some sort of key to the payload that informs Looking Glass what kind of report it's getting

I'm definitely focused on the hands-on lab so I haven't been thinking about how this impacts other potential uses.