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

Remove @eval from @RemoteFile macro call #76

Merged
merged 1 commit into from
Feb 4, 2022

Conversation

disberd
Copy link
Contributor

@disberd disberd commented Feb 4, 2022

This should fix the way the @RemoteFile macro is called inside the get_iers_eop subfunctions to remove calls to the @eval macro.

The use of @eval is not needed since @RemoteFile has proper macro hygiene and also causes pre-compilation errors when using SatelliteToolbox as dependency and calling get_iers_eop in another Package, see issue #75.

The use of @eval was put here probably because @RemoteFile has two signatures, one that takes a symbol as first argument (used to get the name of the variable name to assign the output to), and another that does not and simply return th output of the RemoteFile call.

Two possible solutions are possible:

  1. Keep using the macro and switch to the correct version without @eval, giving the name _eop_iau2000A (or _eop_iau1980) as additional first argument. This may make the code a bit less clear as there is not a visible assignment to the variable
  2. Directly skip the macro to keep the assignment clear in the code and use the underlying call to the RemoteFile function. This has the drawback that the directory where to download the file (which is automatically computed from the macro) has to be put in the code.

I chose the first approach here as it was the one with less friction, but I am willing to change this PR to use the 2nd approach.

@ronisbr
Copy link
Member

ronisbr commented Feb 4, 2022

Hi @disberd !

Perfect, thanks for the PR.

@ronisbr ronisbr merged commit 728e2de into JuliaSpace:master Feb 4, 2022
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.

2 participants