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

Updated default dp for pretty_num #93

Merged
merged 6 commits into from
Oct 17, 2024

Conversation

mzayeddfe
Copy link
Contributor

Brief overview of changes

  • Changed default dp in pretty_num from 2 to 0
  • Amended tests for pretty_num and pretty_num_table

Why are these changes being made?

To stop the change in behaviour in pretty_num in the v0.6.0. This bug forces 2 dp by default.

Detailed description of changes

  • Reverted tests for pretty_num to original ones and added more tests for dp
  • Amended tests in pretty_num_table to reflect the changes made in pretty num
  • Updated documentation

Issue ticket number/s and link

92

reverted tests for pretty_num to og ones and added more tests in pretty_num

amended tests in pretty_num table

updated documentation
@mzayeddfe mzayeddfe linked an issue Oct 16, 2024 that may be closed by this pull request
Copy link
Contributor

@cjrace cjrace left a comment

Choose a reason for hiding this comment

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

Looks good - seems to behave as expected now!

One minor note to add into the description for the function.

And then also worth making this a patch version update (think that'll be v0.6.1?) and a note in the readme mentioning the change?

R/pretty.R Show resolved Hide resolved
Copy link
Contributor

@cjrace cjrace left a comment

Choose a reason for hiding this comment

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

Nice! All good to merge

@mzayeddfe mzayeddfe merged commit f5bcf08 into main Oct 17, 2024
8 checks passed
@mzayeddfe mzayeddfe deleted the 92-default-pretty_num-behaviour-has-changed branch October 18, 2024 13:05
mzayeddfe added a commit that referenced this pull request Oct 18, 2024
* Create pretty table (#91)

* added suggested solution for pretty_table after testing it and adding more comments

* modified function so it doesn't just take numeric cols so it's more flexible

added tests but still need to add more to them

* changed the code to account for using the latest version of pretty_num

* Improve pretty num (#89) (#90)

* added nsmall args to pretty_num and comma_sep to allow formatting of decimals

* amended the function so it takes multiple values

* updated documenation

* fixed the issue with negative dp being passed to nsmall

* changed code for testing pretty_num

* amended the documentation for comma_sep and changed expected_error to expect_equal in test_pretty_num

* fixed the - dp args problem

added nsmall to the documentation

fixed the lintr styling and complexity errors

* added extra tests to pretty_num

* fixing formtting issues for lint testing

* adding documentation, testing and extra comments for pretty_table

an extra documentation line for pretty_num came up when running workflows

* fixing the example for pretty_table by adding export

* amended the documentation to pretty_table to remove refs to the cols just being numeric, add link to the function family and linked the pretty_num function

* improved the documentation

* changed function name
added data frame test
changed the no rows from a warning to a stop
updated documentation

* updated package version, gave details on what was changed in the news.md and added myself to ctb in the description file

* fixed the bracket mistake in description,
put nsmall is code format and updated documentations

* updated the descriptions file so that i'm no longer stealing rich's academic code

* Updated default dp for pretty_num (#93)

* updated the dp default to be 0

reverted tests for pretty_num to og ones and added more tests in pretty_num

amended tests in pretty_num table

updated documentation

* applying changes to code style to pass lint check

* updated documentation

* updated formatting of documentation for spelling

* more updates to the documentation before updating package versions

* updated documentation and readme
mzayeddfe added a commit that referenced this pull request Nov 18, 2024
* started this function but need more clarification before moving forward

* merge main into z-replace (#95)

* Create pretty table (#91)

* added suggested solution for pretty_table after testing it and adding more comments

* modified function so it doesn't just take numeric cols so it's more flexible

added tests but still need to add more to them

* changed the code to account for using the latest version of pretty_num

* Improve pretty num (#89) (#90)

* added nsmall args to pretty_num and comma_sep to allow formatting of decimals

* amended the function so it takes multiple values

* updated documenation

* fixed the issue with negative dp being passed to nsmall

* changed code for testing pretty_num

* amended the documentation for comma_sep and changed expected_error to expect_equal in test_pretty_num

* fixed the - dp args problem

added nsmall to the documentation

fixed the lintr styling and complexity errors

* added extra tests to pretty_num

* fixing formtting issues for lint testing

* adding documentation, testing and extra comments for pretty_table

an extra documentation line for pretty_num came up when running workflows

* fixing the example for pretty_table by adding export

* amended the documentation to pretty_table to remove refs to the cols just being numeric, add link to the function family and linked the pretty_num function

* improved the documentation

* changed function name
added data frame test
changed the no rows from a warning to a stop
updated documentation

* updated package version, gave details on what was changed in the news.md and added myself to ctb in the description file

* fixed the bracket mistake in description,
put nsmall is code format and updated documentations

* updated the descriptions file so that i'm no longer stealing rich's academic code

* Updated default dp for pretty_num (#93)

* updated the dp default to be 0

reverted tests for pretty_num to og ones and added more tests in pretty_num

amended tests in pretty_num table

updated documentation

* applying changes to code style to pass lint check

* updated documentation

* updated formatting of documentation for spelling

* more updates to the documentation before updating package versions

* updated documentation and readme

* created basic components of the function and adding data used for col names

* add excluded and included cols args to the function and added a test

need to add more tests and check the need for included columns

* removed include cols arg, updated documentation and tests

* updated the replacement var for the lintr check and updated documentation for z_replace and vectors

* removing the extra space for pkgdown

* replaced the two vectors with one, updated them and added a script of how to obtain them in data raw

amended test to use microbenchs and to align with the new function

added a check for existence of columns but not format match

* added more tests for error checks.
updated documentation to add microbench temporaily.
deleted geog matrix csv and changed the data raw script so that the vectors are just there manually.
updated pkgdown.

* amended scripts to include another code style check

* updated the data frame in the test and the documentation for z replace

* updated package version to a development one and updated news.

replaced the dataframe used in examples and some of the tests and updated documentation for it.

* checking to see if this makes the microbench print in the logs

* changed cat to print to see if it prints in the logs

* removed microbenchmark code lines in test and dependencies. the time was still less than 0.25 seconds in the logs so lowered to 0.25

* fixed the spelling error

* amended the text for links in documentation so they're more accessible and updated the documentation
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.

Default pretty_num() behaviour has changed
2 participants