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

Don't fail to parse urls from warning messages that include repeated warnings. #12267

Merged
merged 13 commits into from Nov 10, 2021

Conversation

mjkkirschner
Copy link
Member

@mjkkirschner mjkkirschner commented Nov 9, 2021

Purpose

This PR fixes two issues with the documentation browser:

  1. DereferencingANonePointer exception was mis spelled.

  2. Sometimes during delta execution warnings will have different expressionIDs assigned for certain nodes that are composed of multiple expressions - like a multi-output node.

The end result is that the warning text is composed of multiple warnings like:

Dereferencing null pointer - DereferencingNonePointer.html/n Dereferencing null pointer - DereferencingNonePointer.html/n Dereferencing null pointer - DereferencingNonePointer.html

The code that parses url's expected that all warning strings would only be composed of a single single url - instead we make the splitting a bit smarter, by also using the same delimiter that is used to join the warnings.

This code returns the first url found.

TODO

  • tests

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

Release Notes

Fixed a bug where documentation for errors could not be found, or reported illegal characters in the documentation browser.

Copy link
Contributor

@sm6srw sm6srw left a comment

Choose a reason for hiding this comment

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

LGTM!

@pinzart90
Copy link
Contributor

@mjkkirschner do you think that other parts of dynamo could misbehave because of the Delta execution warnings will have different expressionIDs issue ?

Copy link
Contributor

@pinzart90 pinzart90 left a comment

Choose a reason for hiding this comment

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

LGTM

@mjkkirschner
Copy link
Member Author

@pinzart90 - I'm not sure, I actually think that the delta execution behavior is correct, and the first execution only containing a single expressionID is incorrect - it just happened that this masks the documentation issue when testing simple interactions.

@mjkkirschner mjkkirschner merged commit 790d5c7 into DynamoDS:master Nov 10, 2021
@mjkkirschner mjkkirschner deleted the docsug branch November 10, 2021 16:30
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.

None yet

3 participants