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

Improve Files module #220

Merged
merged 16 commits into from
Feb 16, 2018
Merged

Improve Files module #220

merged 16 commits into from
Feb 16, 2018

Conversation

smarr
Copy link
Owner

@smarr smarr commented Feb 11, 2018

The goal of this PR is to improve test coverage, correctness, style, API, and functionality of the Files.ns module.

Main Changes

  • changed file mode to be one of #read, #write, #readWrite, add documentation
  • add method variants that do not take a failure block, but throw exceptions as common defaults
  • fix broken IOException
  • simplify SFileDescriptor
  • make FileTests.ns independent of current directory
  • introduce primitive to get file name from class definition
  • add more tests
  • do not use abbreviated/lower-case class names

Incidental Changes

  • added ClassDefinitionMirror>>#filePath to be able to access the file path of a module
  • make som and JaCoCo support robust to different current working directory
  • fixed issue with Dynamic Metrics tests and coverage tracking

@smarr smarr added the enhancement Improves the implementation with something noteworthy label Feb 11, 2018
@smarr smarr added this to the v0.6.0 - Black Diamonds milestone Feb 11, 2018
Repository owner deleted a comment Feb 11, 2018
daumayr and others added 11 commits February 15, 2018 15:42
- fix imports in FileTests, no abbreviations
- added test for the case of file not being found

Signed-off-by: Stefan Marr <[email protected]>
- avoid issues with invoking `som` from other directories

Signed-off-by: Stefan Marr <[email protected]>
- using the path of the module allows us to run the tests from arbitrary directories

Signed-off-by: Stefan Marr <[email protected]>
Setting an invalid access mode is considered an error an should fail as early as possible.

- add documentation of supported access modes.
  Use simple symbols with camelCase spelling, as practice for everything else.
- invalid access mode will raise an ArgumentError on setting it
- make effort to have a nice error in that case

Signed-off-by: Stefan Marr <[email protected]>
Signed-off-by: Stefan Marr <[email protected]>
Signed-off-by: Stefan Marr <[email protected]>
Repository owner deleted a comment Feb 15, 2018
@smarr
Copy link
Owner Author

smarr commented Feb 15, 2018

@daumayr, I did more work on this. Would be good if you could review, especially to know whether you foresee any issues with what you need.

@ghost
Copy link

ghost commented Feb 16, 2018

@daumayr @smarr Hoping to use this for Grace, so chop chop :)

@smarr
Copy link
Owner Author

smarr commented Feb 16, 2018

@Richard-Roberts where's the review then?

Copy link
Contributor

@daumayr daumayr left a comment

Choose a reason for hiding this comment

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

Looks good to me, so much cleaner code when using files when you don't need all those fail blocks.

public open: mode <Symbol> ifFail: err <[:IOException | X def]> ^ <FileDescriptor | X> = (
(* Open file with given access mode.
Access mode can be #read, #write, or #readWrite. *)
public open: mode <Symbol> ifFail: errBlock <[:Symbol | X def]> ^ <FileDescriptor | X> = (
(* Modes: #read, #write, #readWrite *)
Copy link
Contributor

Choose a reason for hiding this comment

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

i guess thats no longer needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • the comment line

Copy link
Owner Author

Choose a reason for hiding this comment

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

Why? Think it is good practice to provide comments to all methods.
Ideally, VS code is going to show them at some point. (Think we only need to track them during parsing, if we don't do it yet, and then show them on the corresponding request. should be pretty simple)

Copy link
Contributor

Choose a reason for hiding this comment

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

i mean the old one, the content is in the new one, so redundant

Copy link
Owner Author

Choose a reason for hiding this comment

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

aaah, now I see it, yes :) thanks

…referably to indicate failure causes in blocks

Signed-off-by: Stefan Marr <[email protected]>
Repository owner deleted a comment Feb 16, 2018
Repository owner deleted a comment Feb 16, 2018
@smarr
Copy link
Owner Author

smarr commented Feb 16, 2018

Going to merge this once CI on GitLab/Brussels finished successfully

@smarr smarr merged commit 1cd639c into smarr:dev Feb 16, 2018
@smarr smarr changed the title Improve File module Improve Files module Feb 16, 2018
@smarr smarr deleted the file-module-improvements branch February 16, 2018 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves the implementation with something noteworthy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants