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

Refactor `engine.report` to accept a `ReportOptions` object [0.1] #2276

Closed
molant opened this issue Apr 18, 2019 · 5 comments

Comments

Projects
None yet
3 participants
@molant
Copy link
Member

commented Apr 18, 2019

Not sure we need to do it in this PR, but it'd be nice to refactor engine.report to take something similar to ReportOptions instead of continuing to add extra parameters...

Originally posted by @antross in #2269

@karansapolia

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

Hello @molant. I would like to give this a shot. Any helpful pointers?

@antross

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

Hey @karansapolia, thanks for taking this on!

The change should be as simple as refactoring engine.report to take a Problem object directly. That way when we add new pieces of data to Problem we no longer have to update this call.

The only caveat is to ensure the current internal caller in HintContext takes on the duty of falling back to a location of -1, -1 if none is provided. Other than that engine.report is already blindly copying properties anyway.

@molant molant added the breaking label May 6, 2019

@antross antross self-assigned this May 6, 2019

antross added a commit to antross/hint that referenced this issue May 6, 2019

@antross antross changed the title Refactor `engine.report` to accept a `ReportOptions` object Refactor `engine.report` to accept a `ReportOptions` object [0.1] May 6, 2019

@molant molant closed this in #2387 May 6, 2019

molant added a commit that referenced this issue May 6, 2019

@karansapolia

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Hello @antross, My sincere apologies for not following up on this. I came back to this yesterday and was about to push my local commits as PR. The next time I am not available or able to follow up with a task I will make sure to let it be known to the team at the earliest. 😞 😢

@molant

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

No worries, usually we would have waited but we want to publish a new version this week with all the breaking changes and that's why @antross did a PR. There are still a few "good first issue" that are not breaking changes in case you want to pick one 😊

Thanks!

@antross

This comment has been minimized.

Copy link
Member

commented May 7, 2019

@karansapolia My apologies as well. I should have checked to ensure you didn't have any work in progress, but I assumed you just hadn't had time to get to this.

As @molant mentioned we'll usually wait anyway, but if a similar situation arises in the future I'll check-in first to ensure we don't duplicate work. Also if you ever have some other in-progress work but need to step away for awhile, feel free to open a draft PR so we can see where it's at.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.