feat: connectors as S7 object

[akselthomsen]

Summary

Changes connectors() and nested_connectors() to be S7 classes for more robust creation and use.
Also does the same datasources() as new internal class.

Changes Made

• Updated connectors() S7 class with built-in validation inheriting from list ensuring that each Connector is still accessed the same way. Metadata and datasources are available as properties.
• datasources() are now used as an internal class used to define the @datasources property in connectors. This also means that the previous (deprecated) datasources() function has been deleted.
• Updated nested_connectors() to also be an S7 class inheriting from list. Now ensures that all elements are either inheriting from connectors or nested_connectors.
• connect() updated to call the new classes correctly.
• Unit tests updated to also use the new classes correctly.

Questions for reviewer:

I have tried to make as few changes to the user interface as possible, but would it make sense to also do
the below changes?

1. Does it make sense to keep the .datasources argument in connectors()? Since it should always be aligned with the list of Connector objects we could just derive it always in the class (as a getter function).
2. Is it correct to just remove the already soft deprecated datasources() function? I just thought it would be too annoying having to call the new class something different just because we already had this function.
3. Should we change connectors utility functions such as extract_metadata() and write_datasources() to be S7 generics? Could also be in a new issue.

Testing

• Added explicit unit testing for connectors() and nested_connectors(), so testing is not so reliant on the integration with connect().
• Updated logging tests to skip when {renv} is not available. This is due to a current bug in the whirl dependency lists, which means renv is not getting installed, leading to these tests failing ([BUG] renv should be Imports whirl#225).
• I do not understand the new MegaLinter error for the kingfisher linter. It looks to be unrelated to this PR, so should be solved separately, but would like to discuss it.

Checklist

• Code changes have been tested
• Documentation has been updated, if applicable
• All automated tests pass
• Coding style and naming conventions have been followed
• The PR is ready for review and merge

[codecov]

Codecov Report

❌ Patch coverage is 96.20253% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
R/connect.R	80.00%	1 Missing ⚠️
R/connectors.R	98.00%	1 Missing ⚠️
R/nested_connectors.R	93.75%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[Cervangirard]

Questions for reviewer:

I have tried to make as few changes to the user interface as possible, but would it make sense to also do the below changes?

1. Does it make sense to keep the .datasources argument in connectors()? Since it should always be aligned with the list of Connector objects we could just derive it always in the class (as a getter function).

yes, to be consistent with metadata and may be we should transform metadata as a S7 object :)

2. Is it correct to just remove the already soft deprecated datasources() function? I just thought it would be too annoying having to call the new class something different just because we already had this function.

I m sure the function was not used ^^ So yes ^^

3. Should we change connectors utility functions such as extract_metadata() and write_datasources() to be S7 generics? Could also be in a new issue.

yes :)

[akselthomsen]

Questions for reviewer:

I have tried to make as few changes to the user interface as possible, but would it make sense to also do the below changes?

1. Does it make sense to keep the .datasources argument in connectors()? Since it should always be aligned with the list of Connector objects we could just derive it always in the class (as a getter function).

yes, to be consistent with metadata and may be we should transform metadata as a S7 object :)

2. Is it correct to just remove the already soft deprecated datasources() function? I just thought it would be too annoying having to call the new class something different just because we already had this function.

I m sure the function was not used ^^ So yes ^^

3. Should we change connectors utility functions such as extract_metadata() and write_datasources() to be S7 generics? Could also be in a new issue.

yes :)

I have updated based on your comments and added the following:

1. extract_metadata() is now S7 generic
2. write_datasources() is also S7 generic now
3. Removed some invalid input tests for write_datasources() since they are now covered by S7 dispatch

[github-actions]

Code coverage

Name	Coverage (%)
connector	91.97
R/cnt_generics.R	38.71
R/connect_utils.R	100.00
R/connect.R	95.69
R/connector_check_ressource.R	96.00
R/connector.R	96.15
R/connectors.R	98.82
R/conts_datasources.R	99.18
R/dbi_backend_tools.R	100.00
R/dbi_methods.R	100.00
R/dbi.R	78.95
R/fs_backend_tools.R	100.00
R/fs_methods.R	92.21
R/fs_read.R	50.00
R/fs_write.R	72.73
R/fs.R	95.65
R/generic_backend.R	96.55
R/logger_add_logs.R	100.00
R/logger_generics.R	86.67
R/logger_log_dbi.R	100.00
R/logger_log_fs.R	100.00
R/nested_connectors.R	93.75
R/use_connector.R	100.00
R/utils_config.R	100.00
R/utils_docs.R	100.00
R/utils_files.R	100.00
R/utils_roxygen.R	0.00

[github-actions]

Github pages

Review the pkgdown webpage for the PR here