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 NoSuchNodeError catch in dst loading #801

Merged
merged 3 commits into from
Sep 8, 2021

Conversation

gondiaz
Copy link
Collaborator

@gondiaz gondiaz commented Sep 1, 2021

This PR solves issue #800. The proposed solution removes the NoSuchNodeError catch in load_dst function.

@gonzaponte
Copy link
Collaborator

The problem with this function is that it is used both by individual users, which may want just a warning (IIRC, this was the original reason), and the cities, which definitely want to crash if something happens to the file.
Two questions:

  1. What should be done with the other exception that is turned into a warning in this very function? It looks like we need to handle that one as well.
  2. Have you considered introducing an argument to the function to combine both behaviors? Something in the lines of ignore_empty/unsafe?

It's been a while since I last had an issue with this function, so other, more frequent users should comment.

@gondiaz
Copy link
Collaborator Author

gondiaz commented Sep 1, 2021

Agree with point 1 that the other warnings should be changed to exceptions, but probably in another dedicated PR.

For point 2: if I understand well the warnings are meant to avoid code breaking in personal analysis notebooks/scripts where the function is used and those people would also be bothered if a new argument is implemented if the default behaviour is to raise an Exception. Also the warning message it outputs "not of kdst type" should be changed.

@gonzaponte
Copy link
Collaborator

but probably in another dedicated PR.

I don't see the need to open another PR, but I don't have a problem with that.

if I understand well the warnings are meant to avoid code breaking in personal analysis notebooks/scripts where the function is used

Yup

those people would also be bothered if a new argument is implemented if the default behaviour is to raise an Exception.

I'm not sure what you are saying here. The new behavior is going to be to raise an exception in any case, right? Anyone using this function will now have to deal with that anyway. The question is whether to force them to catch the exception explicitly, i.e.

try:
    load_dst(...)
except:
    # do something or not

or to add an argument to their calls, which is arguably less intrusive

load_dst(..., ignore_errors=True)

BTW, there is a third way: to have load_dst to always raise exceptions and define load_dst_xxx that handles the exceptions raised by load_dst. This is pretty much the same as the adding-a-new-parameter option.

@gondiaz
Copy link
Collaborator Author

gondiaz commented Sep 1, 2021

I agree that adding the extra karg is the less intrusive and probably the best option. ignore_errrors = False would be the default and people that uses the function should change to ignore_errrors = True if neccessary.

@gondiaz gondiaz requested a review from Aretno September 6, 2021 13:13
Copy link
Collaborator

@gonzaponte gonzaponte left a comment

Choose a reason for hiding this comment

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

Clear and concise. Approved.

@MiryamMV MiryamMV merged commit 8b12882 into next-exp:master Sep 8, 2021
@gondiaz gondiaz deleted the load_dst_bug branch September 9, 2021 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants