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

Clean up modification of dynamic recordables map in models #1007

Merged
merged 1 commit into from Aug 17, 2018

Conversation

@hakonsbm
Copy link
Contributor

@hakonsbm hakonsbm commented Aug 16, 2018

Do changes directly to recordablesMap_, not to a temporary variable. Also reinstate constant in DynamicRecordablesMap. Fixes #996.

@hakonsbm hakonsbm force-pushed the hakonsbm:cleanup_dynamic_recordables_map_mod branch from 077b546 to a6b9607 Aug 16, 2018
@heplesser heplesser requested review from clinssen and stinebuu Aug 16, 2018
Copy link
Contributor

@clinssen clinssen left a comment

@hakonsbm : why do we not need a temporary copy for the DynamicRecordablesMap but do for the State and Parameters objects? Errors in the latter two are recoverable?

@hakonsbm
Copy link
Contributor Author

@hakonsbm hakonsbm commented Aug 16, 2018

@clinssen Yes, if any properties in the dictionary d are wrong, it throws BadProperty without actually changing anything. Parameters and State aren't changed before we know that all properties are correct. However, it is not possible to recover from errors arising from changing the DynamicRecordablesMap.

@heplesser
Copy link
Contributor

@heplesser heplesser commented Aug 16, 2018

To clarify: inserting into the recordables map can only fail if we run out of memory, and then preserving an old state makes no sense. Setting parameters or state, in contrast, will quite frequently raise exceptions when a user provides an illegal parameter value.

Copy link
Contributor

@clinssen clinssen left a comment

OK, makes sense!

@heplesser heplesser merged commit 67d05dc into nest:master Aug 17, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hakonsbm hakonsbm deleted the hakonsbm:cleanup_dynamic_recordables_map_mod branch Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.