chore: Remove unused parameters recursively

[l0b0]

Since orjson.dumps and json.dumps don't support the same options, we
probably don't want to forward arbitrary arguments to them in any case.

Related Issue(s): #331

Description:

PR Checklist:

• Code is formatted (run pre-commit run --all-files)
• Tests pass (run scripts/test)
• Documentation has been updated to reflect changes, if applicable
• This PR maintains or improves overall codebase code coverage.
• Changes are added to the CHANGELOG. See the docs for information about adding to the changelog.

[codecov-commenter]

Codecov Report

Merging #542 (86d9f19) into main (e4059c8) will not change coverage.
The diff coverage is 33.33%.

@@           Coverage Diff           @@
##             main     #542   +/-   ##
=======================================
  Coverage   94.35%   94.35%           
=======================================
  Files          71       71           
  Lines       10409    10409           
  Branches     1089     1089           
=======================================
  Hits         9821     9821           
  Misses        419      419           
  Partials      169      169           

Impacted Files	Coverage Δ
pystac/stac_io.py	80.34% <33.33%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4059c8...86d9f19. Read the comment docs.

[duckontheweb]

Thanks for moving this into a separate issue, I think it warrants its own discussion.

The reason we had these in the first place is to enable developers to customize json_dumps and then be able to pass arguments through save_json without also having to overwrite that method. I created a branch off of this one with a failing test to illustrate an example usage. That failing test is based on using ujson, but another example might be a case where someone wants to customize json_dumps to use the indent argument in certain cases.

I think an argument could be made that it is not that much extra effort to also overwrite save_json in this case and that maybe it's actually preferable to explicitly overwrite any method signatures that need customizing rather than rely on automatically passing arbitrary arguments. We're passing these arbitrary arguments from StacIO.save_json to StacIO.json_dumps, but not to StacIO.write_text, which is also called by that method. If someone adds arguments in a custom StacIO.write_text implementation they would also need to overwrite StacIO.save_json to include those arguments. Why is this required for write_text but not json_dumps?

It would be good to get comments from maintainers of other downstream libraries to see if/how they use this feature.

@matthewhanson @gadomski Are there examples of where this kind of customization is used?

@lossyrob Are there other examples of usages that would require these arbitrary args/kwargs?

[gadomski]

Are there examples of where this kind of customization is used?

Not in core stactools. In the main method override, args and kwargs are not used: https://github.com/stac-utils/stactools/blob/e90826847e5ebaff2884e2e2f881b29bc3491efd/src/stactools/core/io/__init__.py#L23-L30

[l0b0]

The reason we had these in the first place is to enable developers to customize json_dumps and then be able to pass arguments through save_json without also having to overwrite that method. I created a branch off of this one with a failing test to illustrate an example usage. That failing test is based on using ujson, but another example might be a case where someone wants to customize json_dumps to use the indent argument in certain cases.

Why not just always forward *args and **kwargs to the JSON library? That way they are not unused, and as a bonus, someone who needs any of these extra arguments don't need to write their own copy of the method just to pass them through. Like this (without the docstring):

    def json_dumps(self, json_dict: Dict[str, Any], *args: Any, **kwargs: Any) -> str:
        if orjson is not None:
            return orjson.dumps(json_dict, *args, option=orjson.OPT_INDENT_2, **kwargs).decode("utf-8")
        else:
            return json.dumps(json_dict, *args, indent=2, **kwargs)

[duckontheweb]

The reason we had these in the first place is to enable developers to customize json_dumps and then be able to pass arguments through save_json without also having to overwrite that method. I created a branch off of this one with a failing test to illustrate an example usage. That failing test is based on using ujson, but another example might be a case where someone wants to customize json_dumps to use the indent argument in certain cases.

Why not just always forward *args and **kwargs to the JSON library? That way they are not unused, and as a bonus, someone who needs any of these extra arguments don't need to write their own copy of the method just to pass them through. Like this (without the docstring):

    def json_dumps(self, json_dict: Dict[str, Any], *args: Any, **kwargs: Any) -> str:
        if orjson is not None:
            return orjson.dumps(json_dict, *args, option=orjson.OPT_INDENT_2, **kwargs).decode("utf-8")
        else:
            return json.dumps(json_dict, *args, indent=2, **kwargs)

Yes, that totally makes sense! Not sure why we didn't do that in the first place...

[l0b0]

@duckontheweb I've changed the code to take the optional arguments where applicable. For example, orjson.loads only ever takes a single argument, so I haven't changed that call.

[duckontheweb]

Looks great, thanks for helping us iron this out.