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.

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

Missing exception warnings #12

Closed
wants to merge 1 commit into from
Closed

Missing exception warnings #12

wants to merge 1 commit into from

Conversation

nanjekyejoannah
Copy link

@nanjekyejoannah nanjekyejoannah commented Nov 17, 2022 •
edited
Loading

Dont review until this is merged.

@ltratt
Copy link
Member

ltratt commented Nov 17, 2022

This looks fine to me. Please squash.

@nanjekyejoannah
Copy link
Author

I still need to move some rules to ceval from the AST. Please bear with me as I try to understand a better place.

@nanjekyejoannah
Copy link
Author

This will wait for some time, until I move some helper code related to ceval.

@nanjekyejoannah nanjekyejoannah mentioned this pull request Nov 24, 2022
Merged
bors bot added a commit that referenced this pull request Nov 25, 2022
@bors @nanjekyejoannah
Merge #14
48f8eb8 
14: Warn for raise with three compnents r=ltratt a=nanjekyejoannah

This makes sense at the AST hence isolated from the [other exception PR](#12)

Co-authored-by: Joannah Nanjekye <[email protected]>
@ltratt
Copy link
Member

ltratt commented Jan 12, 2023

@nanjekyejoannah Is this one ready for merging now?

@nanjekyejoannah
Copy link
Author

I have one more PR on my list of 68 things before we come back here. I can certainly get like 4 exception warnings merged as part of this PR and leave what what is making this PR wait, but I prefer to wait to handle all exceptions with the helpers in ceval.

I will do that next after these open PRs are merged, just the dict PR, then this is next thing after.

@ltratt ltratt self-assigned this Jan 13, 2023
@ltratt
Copy link
Member

ltratt commented Jan 13, 2023

If you're happy to wait, I'm fine with that!

@ltratt
Copy link
Member

ltratt commented Feb 16, 2023

Where are we with this one?

@nanjekyejoannah
Copy link
Author

I think lets merge this, I am delayng it because of scope. I will commit the scope PR as a separate topic with other things that involve scope e.g exec, etc. I will handle scope as one topic for all things on my list, Some exception warnings, I already separated before, like the 3 component PR.

@ltratt
Copy link
Member

ltratt commented Feb 16, 2023

Please squash.

@nanjekyejoannah
Copy link
Author

Squashed.

@ltratt
Copy link
Member

ltratt commented Feb 16, 2023

bors r+

bors bot added a commit that referenced this pull request Feb 16, 2023
@bors @nanjekyejoannah
Merge #12
1590b13 
12: Missing exception warnings r=ltratt a=nanjekyejoannah

Dont review until [this](#12) is merged.

Co-authored-by: Joannah Nanjekye <[email protected]>
@bors
Copy link

bors bot commented Feb 16, 2023

Build failed:

@nanjekyejoannah nanjekyejoannah mentioned this pull request Feb 16, 2023
Merged
bors bot added a commit that referenced this pull request Feb 16, 2023
@bors @nanjekyejoannah
Merge #24
1f12b38 
24: Warn for exception: move warning to ceval r=ltratt a=nanjekyejoannah

This replaces the old PR: #12
Moved the warning to ceval.

I removed the three component warning because it was committed in an earlier PR here: #14

Co-authored-by: Joannah Nanjekye <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ltratt ltratt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nanjekyejoannah @ltratt