Add csv differ support for empty rows #5864
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe changes include modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SubmissionsController
participant CsvDiffer
User->>SubmissionsController: GET /submissions
SubmissionsController->>CsvDiffer: Parse CSV data
CsvDiffer->>CsvDiffer: Call padded_table
CsvDiffer-->>SubmissionsController: Return padded CSV data
SubmissionsController-->>User: Render submissions
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/helpers/renderers/csv_differ.rb (1)
49-53: LGTM! Consider adding a comment for clarity.The
padded_tablemethod effectively addresses the issue of handling rows with different column counts. It ensures that all rows have the same number of columns by padding shorter rows with empty strings.Consider adding a brief comment explaining the purpose of this method, for example:
# Ensure all rows in the table have the same number of columns by padding with empty strings def padded_table(table) # ... (existing implementation) endtest/controllers/submissions_controller_test.rb (1)
471-479: Good addition, but consider enhancing the test assertionsThe new test case effectively addresses the PR objective of handling CSV files with empty rows. It creates a submission with a relevant SQL query and a complex JSON result, which is good for testing edge cases.
However, consider the following improvements:
Add more specific assertions to verify the content of the response, not just its status. For example, you could check for the presence of certain elements in the rendered HTML that indicate correct handling of empty rows.
There's a minor typo in the test name: "csv`s" should be "csv's".
Here's a suggestion to improve the test:
test 'Should be able to render submissions with csv`s with empty rows' do submission = create :wrong_submission, code: 'SELECT substr(name,4) FROM artist;', result: "{\"accepted\":false,\"status\":\"wrong\",\"description\":\"Test gefaald\",\"groups\":[{\"description\":\"Validatie\",\"badgeCount\":3,\"groups\":[{\"accepted\":false,\"groups\":[{\"description\":\"Resultaten vergelijken\",\"accepted\":false,\"tests\":[{\"expected\":\"name\\n$pyda\\n1000 Ohm\\n1001 Night Society\\n7 Days In Egypt\\nAC-DC\\nAbdullah Chhadeh\\nAboutou Roots\\nAdmiral Freebee\\nAfricanism All Stars\\nAfrikan Boy\\nAfro Celt Sound System\\nAfter Forever\\nAir\\nAlice Fitoussi\\nAlien The DJ\\nAmaryllis Temmerman\\nAmbeon\\nAmina Alaoui\\nAmália Rodrigues\\nAn Pierlé\\nAnathema\\nAni DiFranco\\nAnkata\\nAnn Christy\\nAnne Clark\\nAphex Twin\\nAphex Twin/James\\nAphex Twin/Richard James\\nApolo Novax\\nArbeid Adelt!\\nArithmetic\\nArmand\\nArno\\nArt Of Noise\\nArte'\\nAsian Dub Foundation\\nAtmosfear Featuring Mae B.\\nAwilo feat. James D Train\\nAxelle Red\\nB-52's\\nBUIKA\\nBamada\\nBana Kin Percussions\\nBarbeton L.M.C.\\n\\\"Beach Boys, THE\\\"\\nBeastie Boys\\n\\\"Beatles, THE\\\"\\nBeautiful Pea Green Boat\\nBeirut Biloma\\nBenny Goodman\\nBenny Goodman \\u0026 His Orchestra\\nBert De Coninck\\nBisso Na Bisso\\nBlack Sabbath\\nBlondie\\n\\\"Boerenzonen op Speed, THE\\\"\\nBoni Gnahoré\\nBoudewijn de Groot\\nBoudewijn deGroot\\n\\\"Boyz from Brazil, THE\\\"\\nBoz Daya\\nBram Vermeulen\\nBuena Vista Social Club\\nCabaret Voltaire\\nCain Principle\\nCamouflage\\nCaptain Beefheart \\u0026 the Magic Band\\nCazima\\nChandeen\\n\\\"Charlatans, THE\\\"\\nCheikha Djenia\\nCheikha Remitti\\n\\\"Chemical Brothers, THE\\\"\\nClotaire K\\nColdplay\\nCrash Course In Science\\nCristina Vilallonga\\n\\\"Cure, THE\\\"\\nD.A.F.\\nDaan\\nDaby Touré\\nDavid Bowie\\nDavid Walters\\nDe Elegasten\\nDe Kreuners\\nDe Mens\\nDe Nieuwe Snaar\\nDe/Vision\\nDead Can Dance\\nDelerium\\nDella Bosiers\\nDella Bossiers\\nDepeche Mode\\nDimitri Van Toren\\nDire Straits\\nDoe Maar\\nEden\\nEl Fish\\nElie Attieh\\nEurythmics\\n\\\"Eurythmics, THE\\\"\\nEva De Roovere\\nEva de Roovere \\u0026 Gerry de Mol\\nExecutive Slacks\\nFad Gadget\\nFadela\\nFaith No More\\nFaithless\\nFats\\nFaudel\\nFernando Maurício\\nFischer-Z\\nFlash \\u0026 the pan\\nFrank Zappa\\nFrans Halsema \\u0026 Jenny Arean\\nFreestylers\\nFront 242\\nFulgence Compaoré\\nGadji Celi\\nGarbage\\nGary Numan\\n\\\"Gathering, THE\\\"\\nGhalia Benali\\nGoldfrapp\\nGorky\\nGotan Project\\nGrauzone\\nGuns N' Roses\\nGuru / DC Lee\\nGuy Manoukian\\nHans de Booij\\nHarem\\nHerman Brood \\u0026 Henny Vrienten\\nHoover\\nHooverphonic\\nHouria\\nHouse Of Pain\\nHugo Raspoet\\nHuman League\\nINXS\\nIggy Pop\\nIke \\u0026 Tina Turner\\nIsigq Samazulu\\nIvan Heylen\\nJan De Wilde\\nJan Puimege\\nJanis Joplin\\nJasmine Yee\\nJeff Buckley\\nJimi Hendrix Experience\\nJo Lemaire\\nJoelle Ursull\\nJohan Verminnen\\nJules De Corte\\nK's choice\\nKadril\\nKevin Mfinka\\nKim Wilde\\n\\\"Kinks, THE\\\"\\nKirpi\\nKoffi Olomide\\nKraftwerk\\nKris de Bruyne\\nLacuna Coil\\n\\\"Lamp, Lazarus \\u0026 Kris\\\"\\nLa´s\\nLed Zeppelin\\nLeftfield\\nLeki\\nLeonard Cohen\\nLes Parents Du Campus\\nLeslie feat Magic system\\u0026Sweety\\nLiesbeth List\\nLiesbeth List \\u0026 Ramses Shaffy\\nLine monty\\nLiquid Liquid\\nLive\\nLiving Colour\\nLouis Neefs\\nLove Is Colder than Death\\nLura\\nM.I.A.\\nMadonna\\nMagic System\\nMamany Kouyaté\\nMarc Moulin\\nMarianne Faithfull\\nMariza\\nMark Knopfler\\nMassive Attack\\nMeiway\\nMelike\\nMelonie Cannon\\nMercan Dede\\nMetallica\\nMidnight Oil\\nMiek en Roel\\nMiel Cools\\nMísia\\nNadia Ben Youcef\\nNatacha Atlas\\nNeeka\\nNeil Young\\nNew Order\\nNick Kamen\\nNightwish\\nNirvana\\nNitzer Ebb\\nNorah Jones\\nNoria\\n\\\"Normal, THE\\\"\\nNorthern Territories\\nNovastar\\nOasis\\nOblomov\\nOrient Funk\\nOsane\\nOuerdia\\nPJ Harvey\\nPapa Wemba\\nPascal DanaÚ\\nPatti Smith Group\\nPaul Van Vliet\\nPearl Jam\\nPendulum\\nPeter Schaap\\nPetit Yode \\u0026 L'Enfant Siro\\nPink Floyd\\nPixies\\n\\\"Police, THE\\\"\\nPraga Khan\\nPrefab Sprout\\n\\\"Pretenders, THE\\\"\\nPrimus\\nPrince\\nPsyche\\nPépé Kallé\\nQueen\\nR.E.G. Project\\nR.E.M.\\nRUM\\nRabah Khalfa\\nRadiohead\\nRage Against the Machine\\nRamses Shaffy\\nRaymond van het Groenewoud\\nRed Zebra\\nReinette l'Oranaise\\nRob De Nijs\\nSabahat Akkiraz\\nSaid M'Rad\\nSalif Keita\\nSam Mangwana\\nSans Papiers feat. Charlotte Mbango\\nSantana\\nSantana/Gypsy Queen\\nSara Tavares\\nSchriekback\\nScorpions\\nShape of Despair\\nSilence Gift\\nSilke Bischof\\nSimon \\u0026 Garfunkel\\nSimon and Garfunkel\\nSimple Minds\\nSisters of Mercy\\n\\\"Sisters of Mercy, THE\\\"\\nSkunk anansie\\nSmashing Pumpkins\\n\\\"Smashing Pumpkins, THE\\\"\\nSnowy Red\\nSois Belle\\nSouad\\nSouad Massi\\nSoukous Stars (Lokassa)\\nSoukous Stars (Zitany Neil)\\nSoul Swirling Somewhere\\nSoulsister\\nSpandau ballet\\nStereo Action Unlimited\\nStijn\\nSubcomandante Marcos\\nSusheela Raman\\nTalking Heads\\nTambours Du Burundi\\nTaoues\\nTechnotronic\\nTerence Trent d'Arby\\nTexas\\nTheatre of Tragedy\\nTherapy?\\nThé Lau \\u0026 Sarah Bettens\\nTori Amos\\nTransglobal Underground\\nTristania\\nTwice A Man\\nU2\\nUnderworld\\nUrbanus\\nVarious Artists\\nVaya Con Dios\\nVicious Pink\\nWalter De Buck\\nWannes Van De Velde\\nWarda\\n\\\"Waterboys, THE\\\"\\nWhite Rose Transmission\\n\\\"White Stripes, THE\\\"\\nWill Ferdy\\nWillem Vermandere\\nWim De Craene\\nWim Sonneveld\\nWithin Temptation\\nYasmina\\nYello\\nZjef Vanuytsel\\nZulu Bamba\\ndEUS\\nlieven tavernier\\nolla vogala\\n\\\"scene, THE\\\"\",\"format\":\"csv\",\"messages\":[{\"format\":\"callout-danger\",\"description\":\"Een of meerdere verwachte kolommen ontbreken. De ingediende query heeft 0 (verplichte) kolommen terwijl er 1 kolommen nodig zijn. De ontbrekende kolommen zijn: name\"}],\"generated\":\"substr\\n Lau \\u0026 Sarah Bettens\\nanus\\nter De Buck\\nda\\nl Ferdy\\nlem Vermandere\\n De Craene\\n Sonneveld\\nmina\\nf Vanuytsel\\nen The DJ\\nlia Rodrigues\\nex Twin\\nand\\nt De Coninck\\nm Vermeulen\\nikha Djenia\\nn\\nid Bowie\\nla Bossiers\\n De Roovere\\nnk Zappa\\nji Celi\\nu / DC Lee\\nria\\ny Pop\\ngq Samazulu\\nis Joplin\\nlle Ursull\\nril\\n Wilde\\ns de Bruyne\\nsbeth List\\na\\n.A.\\nc Moulin\\nk Knopfler\\ncan Dede\\nia Ben Youcef\\nah Jones\\ncal DanaÚ\\nit Yode \\u0026 L'Enfant Siro\\nses Shaffy\\nahat Akkiraz\\ntana\\na Tavares\\nad\\njn\\nues\\ni Amos\\nnes Van De Velde\\nlle Red\\nullah Chhadeh\\nikan Boy\\nce Fitoussi\\nryllis Temmerman\\nna Alaoui\\nPierlé\\n DiFranco\\n Christy\\nex Twin/James\\nex Twin/Richard James\\no\\ni Gnahoré\\ndewijn de Groot\\ndewijn deGroot\\nKA\\nndeen\\nikha Remitti\\ntaire K\\nstina Vilallonga\\ny Touré\\nid Walters\\nla Bosiers\\nitri Van Toren\\ne Attieh\\n de Roovere \\u0026 Gerry de Mol\\ndel\\nnando Maurício\\nns Halsema \\u0026 Jenny Arean\\ngence Compaoré\\ny Numan\\nlia Benali\\n Manoukian\\ns de Booij\\nman Brood \\u0026 Henny Vrienten\\no Raspoet\\n \\u0026 Tina Turner\\nn Heylen\\n De Wilde\\n Puimege\\nmine Yee\\nf Buckley\\nLemaire\\nan Verminnen\\nes De Corte\\nin Mfinka\\npi\\nfi Olomide\\ni\\nnard Cohen\\nlie feat Magic system\\u0026Sweety\\nsbeth List \\u0026 Ramses Shaffy\\nven tavernier\\nis Neefs\\nonna\\nany Kouyaté\\nianne Faithfull\\niza\\nike\\nonie Cannon\\nk en Roel\\nl Cools\\nia\\nacha Atlas\\nl Young\\nk Kamen\\nia\\na Wemba\\nl Van Vliet\\ner Schaap\\nHarvey\\nfab Sprout\\nnce\\né Kallé\\nah Khalfa\\nmond van het Groenewoud\\nnette l'Oranaise\\n De Nijs\\nd M'Rad\\nif Keita\\n Mangwana\\ntana/Gypsy Queen\\nke Bischof\\non \\u0026 Garfunkel\\non and Garfunkel\\nad Massi\\nheela Raman\\nking Heads\\nbours Du Burundi\\nhnotronic\\nence Trent d'Arby\\nas\\n Boerenzonen op Speed\\n Boyz from Brazil\\n Charlatans\\n Chemical Brothers\\n Gathering\\n Kinks\\n Normal\\n Police\\n Sisters of Mercy\\n Smashing Pumpkins\\n Waterboys\\nrapy?\\nnsglobal Underground\\nstania\\nce A Man\\n\\nerworld\\nious Artists\\nte Rose Transmission\\nhin Temptation\\nlo\\nu Bamba\\nwy Red\\ns Belle\\nndau ballet\\nreo Action Unlimited\\n Beach Boys\\n Beatles\\n Cure\\n Eurythmics\\n Pretenders\\n scene\\n White Stripes\\natre of Tragedy\\na Con Dios\\nious Pink\\nda\\n0 Ohm\\n1 Night Society\\nays In Egypt\\nutou Roots\\nDC\\niral Freebee\\nicanism All Stars\\no Celt Sound System\\ner Forever\\n\\neon\\nthema\\nata\\ne Clark\\nlo Novax\\neid Adelt!\\nthmetic\\n Of Noise\\ne'\\nan Dub Foundation\\nosfear Featuring Mae B.\\nlo feat. James D Train\\n2's\\nada\\na Kin Percussions\\nbeton L.M.C.\\nstie Boys\\nutiful Pea Green Boat\\nrut Biloma\\nny Goodman\\nny Goodman \\u0026 His Orchestra\\nso Na Bisso\\nck Sabbath\\nndie\\n Daya\\nna Vista Social Club\\naret Voltaire\\nn Principle\\nouflage\\ntain Beefheart \\u0026 the Magic Band\\nima\\ndplay\\nsh Course In Science\\n.F.\\nElegasten\\nKreuners\\nMens\\nNieuwe Snaar\\nVision\\nd Can Dance\\nerium\\neche Mode\\nS\\ne Straits\\n Maar\\nn\\nFish\\nythmics\\ncutive Slacks\\n Gadget\\nela\\nth No More\\nthless\\ns\\ncher-Z\\nsh \\u0026 the pan\\nestylers\\nnt 242\\nbage\\ndfrapp\\nky\\nan Project\\nuzone\\ns N' Roses\\nem\\nver\\nverphonic\\nse Of Pain\\nan League\\nS\\ni Hendrix Experience\\nftwerk\\n choice\\ns\\nuna Coil\\n\\\"p, Lazarus \\u0026 Kris\\\"\\n Zeppelin\\ntfield\\n Parents Du Campus\\ne monty\\nuid Liquid\\ne\\ning Colour\\ne Is Colder than Death\\nic System\\nsive Attack\\nway\\nallica\\nnight Oil\\nka\\n Order\\nhtwish\\nvana\\nzer Ebb\\nthern Territories\\nastar\\nis\\nomov\\na vogala\\nent Funk\\nne\\nrdia\\nti Smith Group\\nrl Jam\\ndulum\\nk Floyd\\nies\\nga Khan\\nmus\\nche\\nen\\n.G. Project\\n.M.\\niohead\\ne Against the Machine\\n Zebra\\n\\ns Papiers feat. Charlotte Mbango\\nriekback\\nrpions\\npe of Despair\\nence Gift\\nple Minds\\nters of Mercy\\nnk anansie\\nshing Pumpkins\\nkous Stars (Lokassa)\\nkous Stars (Zitany Neil)\\nl Swirling Somewhere\\nlsister\\ncomandante Marcos\",\"accepted\":false}]}]},{\"accepted\":false,\"groups\":[{\"description\":\"Data types vergelijken\",\"accepted\":false,\"tests\":[{\"expected\":\"column,type\\nname,VARCHAR\",\"format\":\"csv\",\"generated\":\"column,type\\nsubstr,VARCHAR\",\"accepted\":false}]}]},{\"accepted\":false,\"groups\":[{\"description\":\"Rij volgorde vergelijken\",\"accepted\":false,\"tests\":[{\"expected\":\"correcte volgorde\",\"messages\":[{\"format\":\"callout-danger\",\"description\":\"De volgorde van de rijen is niet correct. Bekijk nauwgezet de opgave zodat je zeker niets gemist hebt. Om op een betrouwbare manier het verschil tussen de indiening en de modeloplossing te tonen, is de volgorde van rijen op dit tabblad niet (altijd) gelijk aan de volgorde die jij hebt opgegeven. Op het tabblad 'Volledig Resultaat' kan je de exacte resultaten (en volgorde) van je ingediende query bekijken.\"}],\"generated\":\"incorrecte volgorde\",\"accepted\":false}]}]}]},{\"description\":\"Volledig Resultaat\",\"badgeCount\":0,\"groups\":[{\"accepted\":true,\"groups\":[{\"description\":\"Volledig query resultaat\",\"accepted\":true,\"tests\":[{\"expected\":\"\",\"format\":\"csv\",\"generated\":\"substr\\n Lau \\u0026 Sarah Bettens\\nanus\\nter De Buck\\nda\\nl Ferdy\\nlem Vermandere\\n De Craene\\n Sonneveld\\nmina\\nf Vanuytsel\\nen The DJ\\nlia Rodrigues\\nex Twin\\nand\\nt De Coninck\\nm Vermeulen\\nikha Djenia\\nn\\nid Bowie\\nla Bossiers\\n De Roovere\\nnk Zappa\\nji Celi\\nu / DC Lee\\nria\\ny Pop\\ngq Samazulu\\nis Joplin\\nlle Ursull\\nril\\n Wilde\\ns de Bruyne\\nsbeth List\\na\\n.A.\\nc Moulin\\nk Knopfler\\ncan Dede\\nia Ben Youcef\\nah Jones\\ncal DanaÚ\\nit Yode \\u0026 L'Enfant Siro\\nses Shaffy\\nahat Akkiraz\\ntana\\na Tavares\\nad\\njn\\nues\\ni Amos\\nnes Van De Velde\\nlle Red\\nullah Chhadeh\\nikan Boy\\nce Fitoussi\\nryllis Temmerman\\nna Alaoui\\nPierlé\\n DiFranco\\n Christy\\nex Twin/James\\nex Twin/Richard James\\no\\ni Gnahoré\\ndewijn de Groot\\ndewijn deGroot\\nKA\\nndeen\\nikha Remitti\\ntaire K\\nstina Vilallonga\\ny Touré\\nid Walters\\nla Bosiers\\nitri Van Toren\\ne Attieh\\n de Roovere \\u0026 Gerry de Mol\\ndel\\nnando Maurício\\nns Halsema \\u0026 Jenny Arean\\ngence Compaoré\\ny Numan\\nlia Benali\\n Manoukian\\ns de Booij\\nman Brood \\u0026 Henny Vrienten\\no Raspoet\\n \\u0026 Tina Turner\\nn Heylen\\n De Wilde\\n Puimege\\nmine Yee\\nf Buckley\\nLemaire\\nan Verminnen\\nes De Corte\\nin Mfinka\\npi\\nfi Olomide\\ni\\nnard Cohen\\nlie feat Magic system\\u0026Sweety\\nsbeth List \\u0026 Ramses Shaffy\\nven tavernier\\nis Neefs\\nonna\\nany Kouyaté\\nianne Faithfull\\niza\\nike\\nonie Cannon\\nk en Roel\\nl Cools\\nia\\nacha Atlas\\nl Young\\nk Kamen\\nia\\na Wemba\\nl Van Vliet\\ner Schaap\\nHarvey\\nfab Sprout\\nnce\\né Kallé\\nah Khalfa\\nmond van het Groenewoud\\nnette l'Oranaise\\n De Nijs\\nd M'Rad\\nif Keita\\n Mangwana\\ntana/Gypsy Queen\\nke Bischof\\non \\u0026 Garfunkel\\non and Garfunkel\\nad Massi\\nheela Raman\\nking Heads\\nbours Du Burundi\\nhnotronic\\nence Trent d'Arby\\nas\\n Boerenzonen op Speed\\n Boyz from Brazil\\n Charlatans\\n Chemical Brothers\\n Gathering\\n Kinks\\n Normal\\n Police\\n Sisters of Mercy\\n Smashing Pumpkins\\n Waterboys\\nrapy?\\nnsglobal Underground\\nstania\\nce A Man\\n\\nerworld\\nious Artists\\nte Rose Transmission\\nhin Temptation\\nlo\\nu Bamba\\nwy Red\\ns Belle\\nndau ballet\\nreo Action Unlimited\\n Beach Boys\\n Beatles\\n Cure\\n Eurythmics\\n Pretenders\\n scene\\n White Stripes\\natre of Tragedy\\na Con Dios\\nious Pink\\nda\\n0 Ohm\\n1 Night Society\\nays In Egypt\\nutou Roots\\nDC\\niral Freebee\\nicanism All Stars\\no Celt Sound System\\ner Forever\\n\\neon\\nthema\\nata\\ne Clark\\nlo Novax\\neid Adelt!\\nthmetic\\n Of Noise\\ne'\\nan Dub Foundation\\nosfear Featuring Mae B.\\nlo feat. James D Train\\n2's\\nada\\na Kin Percussions\\nbeton L.M.C.\\nstie Boys\\nutiful Pea Green Boat\\nrut Biloma\\nny Goodman\\nny Goodman \\u0026 His Orchestra\\nso Na Bisso\\nck Sabbath\\nndie\\n Daya\\nna Vista Social Club\\naret Voltaire\\nn Principle\\nouflage\\ntain Beefheart \\u0026 the Magic Band\\nima\\ndplay\\nsh Course In Science\\n.F.\\nElegasten\\nKreuners\\nMens\\nNieuwe Snaar\\nVision\\nd Can Dance\\nerium\\neche Mode\\nS\\ne Straits\\n Maar\\nn\\nFish\\nythmics\\ncutive Slacks\\n Gadget\\nela\\nth No More\\nthless\\ns\\ncher-Z\\nsh \\u0026 the pan\\nestylers\\nnt 242\\nbage\\ndfrapp\\nky\\nan Project\\nuzone\\ns N' Roses\\nem\\nver\\nverphonic\\nse Of Pain\\nan League\\nS\\ni Hendrix Experience\\nftwerk\\n choice\\ns\\nuna Coil\\n\\\"p, Lazarus \\u0026 Kris\\\"\\n Zeppelin\\ntfield\\n Parents Du Campus\\ne monty\\nuid Liquid\\ne\\ning Colour\\ne Is Colder than Death\\nic System\\nsive Attack\\nway\\nallica\\nnight Oil\\nka\\n Order\\nhtwish\\nvana\\nzer Ebb\\nthern Territories\\nastar\\nis\\nomov\\na vogala\\nent Funk\\nne\\nrdia\\nti Smith Group\\nrl Jam\\ndulum\\nk Floyd\\nies\\nga Khan\\nmus\\nche\\nen\\n.G. Project\\n.M.\\niohead\\ne Against the Machine\\n Zebra\\n\\ns Papiers feat. Charlotte Mbango\\nriekback\\nrpions\\npe of Despair\\nence Gift\\nple Minds\\nters of Mercy\\nnk anansie\\nshing Pumpkins\\nkous Stars (Lokassa)\\nkous Stars (Zitany Neil)\\nl Swirling Somewhere\\nlsister\\ncomandante Marcos\",\"accepted\":true}]}]}]}],\"messages\":[{\"format\":\"html\",\"description\":\"\\u003cstrong\\u003eWorker:\\u003c/strong\\u003e tantalus\",\"permission\":\"zeus\"},{\"format\":\"html\",\"description\":\"\\u003cstrong\\u003eMemory usage:\\u003c/strong\\u003e 44.05 MiB\",\"permission\":\"zeus\"},{\"format\":\"html\",\"description\":\"\\u003cstrong\\u003ePrepare:\\u003c/strong\\u003e 0.19 seconds\",\"permission\":\"zeus\"},{\"format\":\"html\",\"description\":\"\\u003cstrong\\u003eRuntime:\\u003c/strong\\u003e 0.92 seconds\",\"permission\":\"zeus\"},{\"format\":\"html\",\"description\":\"\\u003cstrong\\u003eResult construction:\\u003c/strong\\u003e 0.01 seconds\",\"permission\":\"zeus\"},{\"format\":\"html\",\"description\":\"\\u003cstrong\\u003eFinalize:\\u003c/strong\\u003e 0.03 seconds\",\"permission\":\"zeus\"},{\"format\":\"html\",\"description\":\"\\u003cstrong\\u003eTotal time:\\u003c/strong\\u003e 1.25 seconds\",\"permission\":\"zeus\"}]}" sign_in submission.user get submission_path(submission) assert_response :ok + assert_select 'table.table-striped' # Check for the presence of a striped table + assert_select 'tr', minimum: 2 # Ensure there are at least two rows (header + data) + assert_select 'td', 'substr' # Check for the presence of 'substr' in a table cell end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- app/helpers/renderers/csv_differ.rb (2 hunks)
- test/controllers/submissions_controller_test.rb (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
app/helpers/renderers/csv_differ.rb (1)
Line range hint
49-66: Summary: Effective solution for handling empty rows in CSV filesThe changes introduced in this PR successfully address the issue of handling empty rows in CSV files. The new
padded_tablemethod ensures that all rows have the same number of columns, which is crucial for the CSV differ functionality. The integration of this method in theinitializefunction guarantees that both generated and expected CSV data are properly padded before further processing.These modifications align well with the PR objectives and provide a clean, minimal solution to the problem. The implementation is efficient and doesn't introduce any apparent issues or side effects.
Great job on implementing this feature! The changes look good and should resolve the bug related to empty lines in CSV files.
Comment on lines
+65
to
+66
| @generated = padded_table(CSV.parse((generated || '').lstrip, nil_value: '')) | ||
| @expected = padded_table(CSV.parse((expected || '').lstrip, nil_value: '')) |
There was a problem hiding this comment.
💡 Codebase verification
Additional CSV.parse Instances Found Require Attention
The following files contain CSV.parse without the padded_table method applied:
test/controllers/evaluations_controller_test.rbapp/helpers/renderers/csv_differ.rb(already addressed)
Please update these instances to use padded_table to maintain consistency in handling CSV data across the codebase.
🔗 Analysis chain
LGTM! Changes align with PR objectives.
The integration of the padded_table method in the initialize method effectively addresses the issue of handling empty rows in CSV files. By applying this method to both @generated and @expected CSV data, we ensure that all rows have the same number of columns before further processing.
To further verify the impact of these changes, we can run the following script to check for any other occurrences of CSV parsing in the codebase that might benefit from similar padding:
This will help us ensure consistency across the codebase in handling CSV data with potential empty rows.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other CSV parsing instances that might need padding
# Test: Look for CSV parsing in Ruby files
rg --type ruby 'CSV\.parse' -C 3
Length of output: 3030
bmesuere
approved these changes
Oct 15, 2024
|
Seeing the large reduction in codecov percentage, something probably went wrong when uploading the coverage results. Happens sometimes |
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request fixes a bug in the csv differ that occurred when empty lines where present.
In the simplified view the diff is displayed by column. For this reason, each row needs the same amount of columns.
To solve this I padded too short rows with empty string