Add support for print to file and file flush. by raff · Pull Request #27 · go-python/gpython · GitHub

No description provided.

Codecov Report

Merging #27 into master will increase coverage by 0.18%.
The diff coverage is 59.45%.

@@            Coverage Diff             @@
##           master      #27      +/-   ##
==========================================
+ Coverage   64.67%   64.86%   +0.18%     
==========================================
  Files          55       55              
  Lines       10044    10074      +30     
==========================================
+ Hits         6496     6534      +38     
+ Misses       3090     3079      -11     
- Partials      458      461       +3

Impacted Files	Coverage Δ
py/file.go	53.14% <55%> (+2.35%)	⬆️
builtin/builtin.go	78.67% <64.7%> (+3.85%)	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8cee534...faf6c54. Read the comment docs.

This looks really good - thank you. Great to have the print working properly.

A few little things inline and a a few queries.

This comment is now out of date :-)

This should be returning an error on failure I think?

I'll see if I can reproduce a flush error in python 3 and fix if needed.

Added check for "file already closed". I don't think flush can return any other error.

I'd say we should as a matter of course return all the errors otherwise tools like errcheck (which I plan to run the codebase through) will complain that we aren't handling errors properly.

The error may need translating, that is for sure, but I think we should always return all errors!

Maybe we should make a generic error translator rather like the one here (from the open code)

        if err != nil {
                // XXX: should check for different types of errors
                switch {
                case os.IsExist(err):
                        return nil, ExceptionNewf(FileExistsError, err.Error())

                case os.IsNotExist(err):
                        return nil, ExceptionNewf(FileNotFoundError, err.Error())
                }
        }

(Note that this particular bit of code is ignoring the err if it wasn't one of those two errors which it shouldn't be - that needs to be fixed)

Maybe this should return None, o.File.Sync() otherwise we are ignoring the error on Sync()

I'll see if I can reproduce a flush error in python 3 and fix if needed.

Likewise here, return None, o.Close()

o.Close() always return None - python File.close() never return an error.

Duplicated __exit__ check?

Sorry, I'll fix it.

Now all failed calls should properly return an error

This looks great now thanks. Will merge :-)

raff force-pushed the print_to_file branch 2 times, most recently from fb4a4f2 to d8a60c1 Compare September 10, 2018 18:58

ncw requested changes Sep 10, 2018

View reviewed changes

raff force-pushed the print_to_file branch 2 times, most recently from e5b841e to 24e39a3 Compare September 11, 2018 15:59

Add support for print to file and file flush.

faf6c54

raff force-pushed the print_to_file branch from 2fc494e to faf6c54 Compare September 11, 2018 16:34

ncw approved these changes Sep 12, 2018

View reviewed changes

ncw merged commit 4f66e54 into go-python:master Sep 12, 2018

raff deleted the print_to_file branch October 4, 2018 20:20

+              	if shouldFlush, _ := py.MakeBool(flush); shouldFlush == py.True {
+              		fflush, err := py.GetAttrString(file, "flush")
+              		if err == nil {
+              			py.Call(fflush, nil, nil)

               // Check interface is satisfied
+              var _ I__enter__ = (*File)(nil)
+              var _ I__exit__ = (*File)(nil)
+              var _ I__exit__ = (*File)(nil)

+              		return nil, err
+              	}
               	// FIXME ignoring file and flush

               }
+              func (o *File) Flush() (Object, error) {
+              	_ = o.File.Sync()

+              func (o *File) M__exit__(exc_type, exc_value, traceback Object) (Object, error) {
+              	o.Close()
+              	return None, nil

Add support for print to file and file flush. by raff · Pull Request #27 · go-python/gpython · GitHub

Uh oh!

Add support for print to file and file flush. - #27

Add support for print to file and file flush.#27ncw merged 1 commit intogo-python:mastergo-python/gpython:masterfrom raff:print_to_fileraff/gpython:print_to_fileCopy head branch name to clipboard

Conversation

raff commented Sep 8, 2018

Uh oh!

codecov-io commented Sep 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ncw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raff commented Sep 11, 2018

Uh oh!

ncw commented Sep 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add support for print to file and file flush.#27
ncw merged 1 commit into
go-python:masterfrom
raff:print_to_file

codecov-io commented Sep 8, 2018 •
edited
Loading