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

Make it able to save constants even if the enum is not resolved (jbevain#886) #889

Closed
wants to merge 1 commit into from

Conversation

Unkorunk
Copy link

No description provided.

@teo-tsirpanis
Copy link
Contributor

Duplicate of #702?

@Unkorunk
Copy link
Author

Unkorunk commented Feb 1, 2023

Not sure. My changes target to add possible save enum even if base types like int can't be resolved, because I think in current version of mono.cecil exist unnecessary extra check for user protection. I think it's good say to users if users do something wrong when it's possible, but approch when we block saving assembly only because user maybe did something wrong maybe didn't, i think it's not good approch.

p.s. this is my subjective opinion. I hope no one gets offended

@teo-tsirpanis
Copy link
Contributor

Both PRs were opened to fix the same bug; the difference is that #702 is more comprehensive and entirely avoids resolving types, while this one provides a fallback in case it was not found.

@ForNeVeR
Copy link

@jbevain, could you please share your thoughts? I believe @teo-tsirpanis's summary above is correct. While you've marked #702 as an unacceptable trade-off, what about this fallback?

@teo-tsirpanis
Copy link
Contributor

you've marked #702 as an unacceptable trade-off

It also doesn't mean that it's not an acceptable trade-off.

He didn't say that, double negation can be confusing. 😅

@ForNeVeR
Copy link

Ah, I'm sorry: I was, indeed, confused by the words there.

@Unkorunk Unkorunk closed this by deleting the head repository Sep 1, 2023
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.

3 participants