View in #core on Slack
@Matt_Robinson: It’s time for the weekly PR review meeting. Who is around to help?
@Simone_Fioravanti: I see that QUnit Tests are back! That’s good!
@Matt_Robinson: It was linked to using
ubuntu-latest (so 22.04) on GitHub and fixed by specifying
ubuntu-20.04 in the action configuration. I have been wondering if I need to file a bug with GitHub as I can’t see reason why it should fail based on the OS.
We have just one PR in the 1.5.0 milestone:
I think we need that one reviewed and merged to avoid the potential for edge case issues in the next release and then we can start to think about a release candidate if other testing to date has gone well.
@Simone_Fioravanti: I’ve just finished to read code changes. Seems good!
@Matt_Robinson: This is linked to the issues I reported last week - I originally thought it was large images, then something Block related but I was failing to reproduce with new images.
Looking at the upstream
blame I found a changeset and from there a ticket that linked the issue to imported data, so that could be a consideration.
Thanks for the PR review, I’ll merge that one shortly then. Anything else for today?
@Simone_Fioravanti: Can we merge #812 so we close something very old?
@Matt_Robinson: Yes, that one looks good. One thing we could consider is a Unit Test.
@Simone_Fioravanti: Unit tests are something I’m not already comfortable with…
@Joy_Reynolds: Yes, I did some work on using the new function
cp_attributes, and I changed the interface a bit for easier usage.
So the original PR that you merged is fine as is, but my new PR changes it to account for the space needed at the front of the attributes.
But that’s not the only thing in that PR…
What do you think for next steps?
@Matt_Robinson: Simone - I’ll take a look at the unit tests.
@Simone_Fioravanti: Thank you!
@Matt_Robinson: Joy, PR is still draft, are you happy with it now?
@Joy_Reynolds: The changes that are there are good, but they aren’t the only ones needed to account for all the front end stuff.
actually, the last one has a coding standards problem that I didn’t know how to solve.
@Simone_Fioravanti: I see all test passing… except for QUnit that just needs to be rerun…
@Joy_Reynolds: As I started on this PR, one of the functions outputs
<li> and the new function generates
<li > so the test didn’t pass. The best way I could see to handle this was to change the function so the attributes can be empty and no space is needed.
@Matt_Robinson: The QUnit fix may need a merge from current
develop to fix that issue, but coding standards all look good.
@Simone_Fioravanti: I’ve re run the tests on #812 and QUnit passed.
@Joy_Reynolds: Oh, I didn’t push the last set because of the coding standards thing…
@Simone_Fioravanti: Sorry I’ve misunderstood!
@Joy_Reynolds: So, what’s in the PR is good, but is a little more than just the original PR to add the function.
should I push what I have?
@Matt_Robinson: If there is more tom come please go ahead and update the PR.
@Joy_Reynolds: OK, that’s 3 more files, and their tests. There’s a coding standards problem.
@Matt_Robinson: Is this needed for 1.5.0?
@Joy_Reynolds: Not “needed”, but follow on from the original PR. Only, once I got into the various usage, I saw that I needed that change to the function you already merged, so in that sense, that part is “needed” for 1.5.0.
I wouldn’t want the function to start wrong and then have to change.
@Matt_Robinson: Okay, so with us being very close to 1.5.0-RC, are we waiting for this or pulling it out?
The plan was maybe for an RC over the weekend.
@Joy_Reynolds: My preference is to cherry pick the changes from my draft PR to fix the function (and tests) of the original PR, so that is part of 1.5.0, but it isn’t wrong…
@Matt_Robinson: Okay so a slight rework of the original - sounds good, is that something you can do as a separate PR?
@Joy_Reynolds: Is there a git flow for that or do I just copy files around myself and make it?
@Matt_Robinson: The latter, make a new branch from develop and make the changes there.
You could perhaps cherry-pick any commits locally that have a commit id.
@Joy_Reynolds: Yeah, I think it’s mixed with the other files that I was enhancing.
I’ll push a new PR later today.
@Matt_Robinson: Great, thanks all, a little closer to the next release.