Skip to content

Fixing the API break that was caused by this PR (https://github.com/DynamoDS/Dynamo/pull/9665) #9728

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.

Sign up for GitHub

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

Jump to bottom
Merged
reddyashish merged 6 commits into from
May 23, 2019
Merged

Fixing the API break that was caused by this PR (https://github.com/DynamoDS/Dynamo/pull/9665) #9728

reddyashish merged 6 commits into from
May 23, 2019

Conversation

@reddyashish
Copy link
Contributor

@reddyashish reddyashish commented May 21, 2019 •
edited
Loading

Purpose

This PR is to address the API break that was introduced by removing the "TraceUtils.cs" file from the ProtoCore project.

JIRA task: https://jira.autodesk.com/browse/DYN-1898

Refer to this change: b859c7b#r33609626

Declarations

Check these if you believe they are true

  • The code base 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.

Reviewers

@aparajit-pratap

@aparajit-pratap
Copy link
Contributor

aparajit-pratap commented May 22, 2019

@reddyashish what about the actual file itself? Is it still checked in into the repo? Why are the build checks failing?

@reddyashish
Copy link
Contributor Author

reddyashish commented May 22, 2019

@aparajit-pratap The actual file was never deleted and verified it is present. The issue was that it was causing ambiguity in "CallSite.cs" as TraceUtils class is present in both ProtoCore.Lang and DynamoServices namespaces. Changed it to use the one from DynamoServices namespace. I will check on the builds again once they finish.

aparajit-pratap
aparajit-pratap reviewed May 22, 2019
View reviewed changes
@aparajit-pratap
Copy link
Contributor

aparajit-pratap commented May 23, 2019

@reddyashish I think this is LGTM as long as you've run all tests to make sure that there are no regressions just in case. Any idea why the appveyor and travis builds are failing on the PR?

@reddyashish
Copy link
Contributor Author

reddyashish commented May 23, 2019

Yes, I have run all the tests and there are no regressions. Not sure why the appveyor and travis builds failed. In the logs, it just shows that the build was cancelled by system administrator. Triggered them again.

@reddyashish
Copy link
Contributor Author

reddyashish commented May 23, 2019

Merging this.

@reddyashish reddyashish merged commit 91c9924 into DynamoDS:master May 23, 2019
mjkkirschner pushed a commit that referenced this pull request Aug 3, 2019
@reddyashish @mjkkirschner
Fixing the API break that was caused by this PR (#9665) (#9728)
4ffcb65 
* Fixing the API break.

* Removing spaces

* Removing spaces

* Removing spaces

* Using the traceUtils.cs class from DynamoServices.

* Spacing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aparajit-pratap aparajit-pratap aparajit-pratap left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@reddyashish @aparajit-pratap