codecheckers / codecheck

R package assistant for conducting a CODECHECK
MIT License
10 stars 3 forks source link

Split register type column #55

Closed angelina-momin closed 1 month ago

angelina-momin commented 2 months ago

Solves issues:

Related register PR #98

In the register.csv the column "type" was split into into "type" and "venue" where Venue = what is in brackets now Type = outside of brackets

Changes made:

angelina-momin commented 2 months ago

@nuest I am redoing the code and thought of dropping the "type" and "venue" column for page of registers by venue since those columns are redundant. Tidyr's nest() function also drops the column by which grouping is done so in case of filter by venue it drops the venue column in the resulting nested tables.

Is that okay with you? The json files will only contain the information showed on the page

Refer to the top screenshot for the new look and the bottom screenshot of the old look.

Screenshot 2024-09-03 at 09 43 21

image

nuest commented 2 months ago

Yes, dropping the column is fine - even for the JSON the information is encoded in the URL.


Can you easily change the order to Certificate, Report, Repository, Issue, Check date ? If extremely easy maybe do it, it more than a minute of work I will create an issue.

It would also be good to get the title in there, I think we have an issue for that..

angelina-momin commented 1 month ago

@nuest The code is ready for review again. Redid a lot of code on filtering the tables following your suggestion to use tidyr. Please refer to the description of the PR for what I changed.

Can you create a separate issue for the JSON column order please? Prefer a separate issue because this PR already combines two PRs

angelina-momin commented 1 month ago

Wow, what a rewrite. Not easy to digest for me, but I find the new code quite readable. What do you think? Are you happier with the new approach?

Yes I am happy with the new approach. Simpler, easier to read and should be more efficient :)

Only had a few minor remarks. I'm fine if you go ahead and merge yourself after the final couple of changes, as I might be a bit busy the next days.

Good job!

Thanks! Just resolved the comments and i'll proceed with merging