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

Expose -Wno via ignore-warnings kompile option #4471

Merged
merged 13 commits into from
Jun 25, 2024
Merged

Expose -Wno via ignore-warnings kompile option #4471

merged 13 commits into from
Jun 25, 2024

Conversation

JuanCoRo
Copy link
Member

Address #4470

@JuanCoRo JuanCoRo requested a review from nwatson22 June 21, 2024 16:30
@rv-jenkins rv-jenkins changed the base branch from master to develop June 21, 2024 16:30
@Baltoli
Copy link
Contributor

Baltoli commented Jun 21, 2024

@JuanCoRo you'll need to rebase out these version noise commits; PRs to K should be based on the develop branch rather than master.

Copy link
Contributor

@Baltoli Baltoli left a comment

Choose a reason for hiding this comment

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

rebase needed

@JuanCoRo
Copy link
Member Author

Thanks @Baltoli! Let me know if this looks good. Forgot to start off develop 😨

@JuanCoRo JuanCoRo marked this pull request as ready for review June 21, 2024 17:57
pyk/src/pyk/ktool/kompile.py Outdated Show resolved Hide resolved
@JuanCoRo JuanCoRo enabled auto-merge (squash) June 21, 2024 21:46
pyk/src/pyk/ktool/kompile.py Outdated Show resolved Hide resolved
@tothtamas28 tothtamas28 disabled auto-merge June 21, 2024 22:08
@@ -206,6 +206,7 @@ class KompileOptions(Options):
bison_lists: bool
no_exc_wrap: bool
outer_parsed_json: bool
ignore_warnings: list[str] | None
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a 100% sure in this, but consider

Suggested change
ignore_warnings: list[str] | None
ignore_warnings: list[str]

This makes it consitent with ccopts.

@nwatson22, is this the right convention to follow?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'd go without the None type option unless there is a situation where it could actually be set to None, which there shouldn't be as far as I know since you have [] as the default.

pyk/src/pyk/cli/args.py Outdated Show resolved Hide resolved
pyk/src/pyk/cli/args.py Outdated Show resolved Hide resolved
pyk/src/pyk/cli/args.py Outdated Show resolved Hide resolved
pyk/src/pyk/ktool/kompile.py Outdated Show resolved Hide resolved
pyk/src/pyk/__main__.py Outdated Show resolved Hide resolved
@JuanCoRo
Copy link
Member Author

@tothtamas28 just addressed all comments above (hopefully). Let me know if there's anything I missed while we wait on @nwatson22's input on removing the | None part of the ignore_warnings type.

@JuanCoRo
Copy link
Member Author

@nwatson22 @tothtamas28 @Baltoli do you see anything missing in this PR? Need this one merged to start merging the upstream ones. Let me know if there's anything else from my side that needs to be done!

@rv-jenkins rv-jenkins merged commit 3bc2373 into develop Jun 25, 2024
18 checks passed
@rv-jenkins rv-jenkins deleted the expose-Wno branch June 25, 2024 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants