Closed jamesmbaazam closed 10 months ago
Thanks for the PR @jamesmbaazam - is this more or less ready to review? I would suggest rebasing it on main
, and triggering the required workflows. Happy to take a brief look until then.
It's incomplete as I'm yet to work on <intervention>
, that's why I've marked it as a draft.
Hi @jamesmbaazam - just a gentle nudge to remind that this draft is still open. Could we aim to close it by Weds next week?
Yes, I'm aware. Except this week is short, so I can't get to it. I'll resume next week. Thanks for the nudge.
Hi @jamesmbaazam, I was taking a sneak-peek at the new printing, and I like it! Just FYI the <infection>
class is going away once I push a fix to #133, so I wouldn't put too much more effort into that one. I liked what you had done with the print method for that one, but we have decided (just yesterday) on a change in direction on that class.
Oh, thanks for the update. It's definitely on my to-do list for the week. I hope to push the rest of the changes and open it up for review hopefully by the close of the week.
@pratikunterwegs I think the current failing checks are not related to this PR so they'll have to be triggered again at some point to get them fixed.
@pratikunterwegs I think the current failing checks are not related to this PR so they'll have to be triggered again at some point to get them fixed.
Thanks @jamesmbaazam, will take a look next week. Agreed, the checks are failing on account of a R setup step.
Locally, and on GHA, the lintr checks still fail due to the 'name' and 'header' variables.
The other solution is to wrap them in #nolint
tags. I've checked and it works. Not sure how you feel about that.
Locally, and on GHA, the lintr checks still fail due to the 'name' and 'header' variables.
The other solution is to wrap them in
#nolint
tags. I've checked and it works. Not sure how you feel about that.
Thanks, it would be good to avoid that - any idea why the solution posted in https://github.com/r-lib/lintr/issues/352 doesn't seem to be working?
Anyway, my solution would be to use {cli} and {glue} to create strings and then use writeLines()
/cat()
to print them. E.g.
name = class(x)
name = cli::col_red(name)
writeLines(name)
See some of the print methods in {scenarios} (on main
) for examples of how I've approached this in the past.
Thanks, it would be good to avoid that - any idea why the solution posted in r-lib/lintr#352 doesn't seem to be working?
Using #nolint
has been suggested in various places including under your linked issue https://github.com/r-lib/lintr/issues/2162#issuecomment-1718341607.
Anyway, my solution would be to use {cli} and {glue} to create strings and then use
writeLines()
/cat()
to print them.
Even in cases where I've passed name
to cat()
, I've still had the same issue.
Anyway, my solution would be to use {cli} and {glue} to create strings and then use
writeLines()
/cat()
to print them. E.g.name = class(x) name = cli::col_red(name) writeLines(name)
I'll try this and see.
Anyway, my solution would be to use {cli} and {glue} to create strings and then use
writeLines()
/cat()
to print them. E.g.name = class(x) name = cli::col_red(name) writeLines(name)
I'll try this and see.
Thanks - it would be great if this or similar works, but otherwise use nolint for now. I'll aim to remove them later.
Thanks @jamesmbaazam - feel free to rebase and merge when you're ready. Note the current option showing is squash-merge. I've created issue #136 if you wish to return to this in future.
This PR addresses #34. The context is outlined in the linked issue.