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.

Sign up for GitHub

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

Jump to bottom

Add Execution Context control to Text.write #6459

Merged
merged 14 commits into from
Apr 29, 2023
Merged

Add Execution Context control to Text.write #6459

merged 14 commits into from
Apr 29, 2023

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Apr 27, 2023 •
edited
Loading

image

Pull Request Description

  • Adjusted Context.is_enabled to support default argument (moved built in so can have defaults).
  • Made environment case-insensitive.
  • Bug fix for play button.
  • Short hand to execute within an enabled context.
  • Forbid file writing if the Output context is disabled with a Forbidden_Operation error.
  • Add temporary file support via File.create_temporary_file which is deleted on exit of JVM.
  • Execution Context first pass in Text.write.
    • Added dry run warning.
    • Writes to a temporary file if disabled.
    • Created a DryRunFileManager which will create and manage the temporary files.
  • Added format dropdown to File.read and Data.read.
  • Renamed JSON_File to JSON_Format to be consistent.

(still to unit test).

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

GregoryTravis
GregoryTravis approved these changes Apr 27, 2023
View reviewed changes
distribution/lib/Standard/Base/0.0.0-dev/src/System/File/Write_Extensions.enso Outdated

is_enabled = Context.Output.is_enabled

if_existing = if is_enabled then on_existing_file else
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this logic might be moved into Existing_File_Behavior.

Copy link
Member Author

@jdunkerley jdunkerley Apr 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

yes, that's exactly what I expect to do as we build it out.

Will be cases we need special handling but this should work for writing text and bytes. Likewise for Table.write with exception of Excel (which is odd).

Copy link
Member

Choose a reason for hiding this comment

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

I agree that it would be good to move this to some common place. But OK with doing that later, once we actually have >1 place using it.

farmaazon
farmaazon requested changes Apr 28, 2023
View reviewed changes
app/gui/src/controller/project.rs Outdated Show resolved Hide resolved
farmaazon
farmaazon approved these changes Apr 28, 2023
View reviewed changes
Copy link
Contributor

@farmaazon farmaazon left a comment

Choose a reason for hiding this comment

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

Accepted rs file

radeusgd
radeusgd reviewed Apr 28, 2023
View reviewed changes
distribution/lib/Standard/Base/0.0.0-dev/src/System/File/Write_Extensions.enso
Existing_File_Behavior.Error -> if actual.exists then Error.throw (File_Error.Already_Exists actual) else Existing_File_Behavior.Overwrite
_ -> on_existing_file

file = if is_enabled then actual else actual.create_dry_run_file copy_original=on_existing_file==Existing_File_Behavior.Append
Copy link
Member

Choose a reason for hiding this comment

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

This is getting complicated. But I appreciate this allows us to have quite sane dry-run behaviour, so I think it's ok. I hope we will not have to have these kinds of complications scattered around all of our write methods, but hopefully will be able to encapsulate it in some single place.

But that's something to be done later, just noting the intentions.

radeusgd
radeusgd approved these changes Apr 28, 2023
View reviewed changes
Copy link
Member

@radeusgd radeusgd 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, I'm glad we will finally be able to test these in practice

@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Apr 28, 2023
@jdunkerley
Prototype Exec Context work
2ae5d57 
- Adjusted Context.is_enabled to support default argument.
- Made environment case-insensitive.
- Add temporary file support.
- Execution Context first pass in Text.write.
- Added format drop down to `read`.
- Renamed JSON_File to JSON_Format to be consistent.
@jdunkerley
More work:
be274b1 
- Add doc for Text.write.
- Low level error for file writing.
- Warning on dry run.
- Fixes for File_Format drop down.
- Fix for play button call.
@jdunkerley
Revert accidental commit.
4d33e4a
@jdunkerley
Adjust so temporary file is only created once for each path allowing …
9f4e7cd 
…for appends.
@jdunkerley
Java Format.
e50fc26
@jdunkerley
ChangeLog.
e25256f
@jdunkerley
- PR comments.
93ef33c 
- Fix encoding drop down.
- Repair test issues from blocking writing operations.
- Allow Examples to write.
@jdunkerley
More failing test repairs.
b19f759
@jdunkerley
More failing test repairs.
b2b3f77 
Fix encoding drop down.
@jdunkerley
Last PR comments.
6d9fbf4
@jdunkerley
Java formatting and failed tests.
af3ddb0
@jdunkerley
GUI linting.
dc935a1
@jdunkerley
Prettier.
7c5bb4c
@jdunkerley
Allow creating output folder.q
34b19c2
@mergify mergify bot merged commit 6b0c682 into develop Apr 29, 2023
@mergify mergify bot deleted the wip/jd/execcontext-test branch April 29, 2023 08:39
Procrat added a commit that referenced this pull request May 2, 2023
@Procrat
Merge branch 'develop' into wip/procrat/home-button-6399
4234c47 
* develop:
  Finishing Vector Editor (#6470)
  Proper handling of multiple list views. (#6461)
  Fix disappearing cached shapes (#6458)
  Add Execution Context control to Text.write (#6459)
  Change defaults for `Connection.tables` and ensure that `Connection.query` recognizes all available tables (#6443)
Procrat added a commit that referenced this pull request May 3, 2023
@Procrat
Merge branch 'develop' into wip/procrat/json-viz-last-row-6196
b63966e 
* develop: (34 commits)
  Continued Execution Context work and some little fixes (#6506)
  IDE's logging to a file (#6478)
  Fix application config (#6513)
  Cloud/desktop mode switcher (#6448)
  Fix doubled named arguments bug (#6422)
  Reimplement `enso_project` as a proper builtin (#6352)
  Fix layer ordering between dropdown and breadcrumbs backgrounds.  (#6483)
  Multiflavor layers (#6477)
  DataflowAnalysis preserves dependencies order (#6493)
  Implement `create_database_table` for Database Table (#6467)
  Limit Dead Letter logging (#6482)
  More reliable shutdown of the EnsoContext to save resources (#6468)
  Make execution mode `live` default for CLI (#6496)
  Finishing Vector Editor (#6470)
  Proper handling of multiple list views. (#6461)
  Fix disappearing cached shapes (#6458)
  Add Execution Context control to Text.write (#6459)
  Change defaults for `Connection.tables` and ensure that `Connection.query` recognizes all available tables (#6443)
  Introducing @BuiltinMethod.needsFrame and InlineableNode (#6442)
  Hide profile button behind a feature flag (#6430)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vitvakatu vitvakatu vitvakatu left review comments

@GregoryTravis GregoryTravis GregoryTravis approved these changes

@farmaazon farmaazon farmaazon approved these changes

@radeusgd radeusgd radeusgd approved these changes

@hubertp hubertp hubertp approved these changes

@4e6 4e6 Awaiting requested review from 4e6 4e6 is a code owner

@JaroslavTulach JaroslavTulach Awaiting requested review from JaroslavTulach JaroslavTulach is a code owner

@MichaelMauderer MichaelMauderer Awaiting requested review from MichaelMauderer

@wdanilo wdanilo Awaiting requested review from wdanilo

@mwu-tow mwu-tow Awaiting requested review from mwu-tow

@kazcw kazcw Awaiting requested review from kazcw kazcw is a code owner

@Akirathan Akirathan Awaiting requested review from Akirathan Akirathan is a code owner

Assignees
No one assigned
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jdunkerley @hubertp @radeusgd @farmaazon @vitvakatu @GregoryTravis