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 the File.open constructor use read-only mode (Issue #2567) #2568

Merged
merged 1 commit into from
Feb 28, 2018

Conversation

tmmcguire
Copy link
Contributor

Use @ponyint_o_rdonly rather than _rdwr in the calls to @OPEN and
@_open.

Use @ponyint_o_rdonly rather than _rdwr in the calls to @OPEN and
@_open.
@slfritchie slfritchie added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Feb 26, 2018
@slfritchie
Copy link
Contributor

I hadn't paid much attention to File's API until this ticket. It seems odd to me (wearing a C programmer's hat) that so little control is available to a Pony programmer for open(2)'s 2nd and 3rd arguments ... but that's a matter for a different PR or perhaps even RFC.

In this case, it's pretty clear to me that this PR fixes a bug. The docstring for File.open says "Open for read only" and also desires read-only behavior from the rest of the class by using writable = false. � Anybody have a different opinion?

@jemc
Copy link
Member

jemc commented Feb 28, 2018

I agree. Should we go ahead and merge this?

@jemc
Copy link
Member

jemc commented Feb 28, 2018

Discussed on sync call.

We can merge this as-is, because it fixes an issue and doesn't make anything worse.

However, we need to spend some time looking at how to handle errors where you open a file with read-only permissions and try to write to it. It appears to me that such writes are silently swallowed at runtime, rather than raising an error at compile time or runtime. @mfelsche is opening an issue to discuss this.

@jemc jemc merged commit e207b5d into ponylang:master Feb 28, 2018
ponylang-main added a commit that referenced this pull request Feb 28, 2018
dipinhora pushed a commit to dipinhora/ponyc that referenced this pull request Jun 5, 2018
Use @ponyint_o_rdonly rather than _rdwr in the calls to @OPEN and
@_open.
dipinhora pushed a commit to dipinhora/ponyc that referenced this pull request Jun 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants