Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor changes #67

Merged
merged 192 commits into from
Mar 31, 2023
Merged

Minor changes #67

merged 192 commits into from
Mar 31, 2023

Conversation

ElsLommelen
Copy link
Collaborator

PR aangemaakt om te testen of codecov het verschil in coverage detecteert. Deze bevat voorlopig een aantal kleine aanpassingen, maar is nog niet afgewerkt.

…owed warnings and notes are grouped together
@ElsLommelen
Copy link
Collaborator Author

Ok, bij deze is het wat mij betreft afgerond, dus laat maar weten als alles goedgekeurd is en de nieuwe versie gepubliceerd mag worden.

@leymanan
Copy link
Collaborator

OK, bedankt!
Ik hoop er eind volgende week ook mee klaar te zijn (met de controles van de uitbreidingen).
Ik zal dan ook nog eens de documentatie doornemen. Ik hou je op de hoogte!

@ElsLommelen
Copy link
Collaborator Author

Ter info: ik heb bij enkele andere functies dezelfde truc toegepast als bij GoedgekeurdeUitbreidingen in validatie.uitbreiding() om te vermijden dat daar gelijkaardige problemen als beschreven in issue #76 zouden kunnen optreden. Dit zou geen invloed mogen hebben op je code, maar dus best toch even de nieuwste versie installeren vooraleer je verder test (of dit na afsluiten of voor opstarten even doen).

Copy link
Collaborator

@leymanan leymanan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Els,

ik heb de documentatie ook nog eens doorgenomen , en voor mij mag dit afgerond worden.
Ziet er heel goed uit, duidelijk en volledig!
Ik heb enkel nog wat kleine suggesties gedaan mbt de handleiding.
Ik wist niet zeker of ik dan "Approve" mocht aanvinken (wordt dan automatisch gemerged?), en "Request changes" klonk dan weer zo hard (wantzijn suggesties ...).
Dus dan "Comment" aangevinkt.

Alvast bedankt!!

Grts, Anja

R/validatie_uitbreiding.R Outdated Show resolved Hide resolved
vignettes/Handleiding.Rmd Outdated Show resolved Hide resolved
vignettes/Handleiding.Rmd Outdated Show resolved Hide resolved
vignettes/Handleiding.Rmd Outdated Show resolved Hide resolved
vignettes/Handleiding.Rmd Outdated Show resolved Hide resolved
vignettes/Handleiding.Rmd Outdated Show resolved Hide resolved
vignettes/Handleiding.Rmd Outdated Show resolved Hide resolved
vignettes/Handleiding.Rmd Outdated Show resolved Hide resolved
vignettes/Handleiding.Rmd Show resolved Hide resolved
@ElsLommelen
Copy link
Collaborator Author

Bedankt om dit nog eens allemaal na te lezen!
En ja hoor, die 'approve' had je mogen aanvinken, dat is enkel om aan te geven dat je akkoord bent dat het gemerged wordt (en je doet hiermee geen merge). Hier heb ik dat niet als vereiste opgegeven, maar in sommige packages is het nodig dat er zo een 'approval' is (of meerdere, bv. 2) vooraleer de branch gemerged kan worden (net zoals ik het hier zo ingesteld heb dat alle testen moeten slagen voor ik kan mergen).

Dus de 3 opties zijn als volgt:

  • approval: je geeft expliciet toestemming om te mergen (dit doe je dus als alles voor u in orde is, maar kan je dus ook al doen als je nog enkele kleine suggesties gegeven hebt die te nemen of te laten zijn)
  • comment: gewoon een commentaar, zonder gevolgen (kan je doen als je nog niet helemaal akkoord bent maar ook niet te ampetant wil doen of het aan anderen wil laten om evt. een approval te geven)
  • 'request changes': da's inderdaad ene harde, want dan ga jij achteraf expliciet nog een 'approval' moeten geven vooraleer er gemerged kan worden, zelfs al hebben meerdere andere personen al een approval gegeven. Dus hiermee kan je echt een merge tegenhouden (misschien wel enkel als een approval als voorwaarde gesteld is voor een merge?).

Enfin, ik wacht nog even af tot de testen gepasseerd zijn, en dan wordt versie 0.2 officieel gepubliceerd. :-)

@ElsLommelen ElsLommelen merged commit a7458d9 into main Mar 31, 2023
@ElsLommelen ElsLommelen deleted the minor-changes branch March 31, 2023 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants