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

Execution control for Table.write and various widget tweaks... #6835

Merged
merged 25 commits into from
Jun 1, 2023

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented May 24, 2023

Pull Request Description

  • Adds execution control to Table.write.
    • Refactored the Text.write to make part reusable.
    • Tidied up some legacy mess in tests.
  • Add easier flow to go from Text to an URI to fetching data.
  • Add decode functions to Response and Response_Body.
  • Fix issue with 0 length regex matches (using same as Python and .Net approach).
  • Add various ALIAS entries to make function discovery easier.
  • Sort a lot of drop down and vector editors out (including switch to fully qualified names).

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.

@jdunkerley jdunkerley force-pushed the wip/jd/table-write-100 branch 3 times, most recently from d74e525 to 1078748 Compare May 31, 2023 13:11
count_not_nothing = Option "Count Not Nothing" "(Aggregate_Column.Count_Not_Nothing)" [column_widget]
count_nothing = Option "Count Nothing" "(Aggregate_Column.Count_Nothing)" [column_widget]
count_not_nothing = Option "Count Not Nothing" fqn+".Count_Not_Nothing" [column_widget]
count_nothing = Option "Count Nothing" fqn+".Count_Nothing" [column_widget]

## Should be a list of Text columns only
Copy link
Member

Choose a reason for hiding this comment

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

Should we start implementing this by-type column filtering? We have all the capabilities needed to do that.

If not in this PR, I'd be happy to create a small PR for this - it's just a few lines of adjustment to get a really nice UX improvement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed with Ned and he argued against it.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, why?

Copy link
Member Author

Choose a reason for hiding this comment

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

Logic was more confusing for columns to vanish depending on the mode.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I see. Well indeed maybe it would be less confusing if we chose the column first and then had a list of aggregations available for it - but that does not fit our Aggregate_Column format.

IMHO suggesting invalid columns for an aggregation is not that helpful, I don't imagine it is a problem, but if that's the recommendation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking the same and wondered around adding a note if an unsupported type - so you would see the same set but something in brackets warning invalid for this).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think that would be good. Or even, if we had the ability to add styling, the invalid types could be greyed out.

Also, I would move all unsupported down to the bottom, so that the first fields that show up are the supported ones. No sense in having to scroll thru unsupported fields to find a good one.

Copy link
Member

Choose a reason for hiding this comment

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

Shall we create a follow up task for this?

I think the sorting will be especially useful, and marking the not-matching ones should also be helpful. Since we don't have formatting yet, maybe we can mark them like /Shortest/?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will add a task.

@jdunkerley jdunkerley force-pushed the wip/jd/table-write-100 branch from faf6e15 to 6d68149 Compare June 1, 2023 15:46
@jdunkerley jdunkerley marked this pull request as ready for review June 1, 2023 17:11
@jdunkerley jdunkerley requested a review from GregoryTravis as a code owner June 1, 2023 17:11
@@ -15,7 +15,8 @@ type Boolean
True
False

## Computes the logical and (conjunction) of two booleans.
## ALIAS And
Computes the logical and (conjunction) of two booleans.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Computes the logical and (conjunction) of two booleans.
Computes the conjunction (logical 'and') of two booleans.

I'm wondering if that form would not be more readable? (low priority)

content_type self =
response_headers = self.headers
content_type = response_headers.find if_missing=Nothing h-> "Content-Type".equals_ignore_case h.name
if content_type.is_nothing then Nothing else content_type.value
Copy link
Member

Choose a reason for hiding this comment

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

not for now, but I'm wondering if we should look for some idiom for this, like Kotlin's elvis operator:

Suggested change
if content_type.is_nothing then Nothing else content_type.value
content_type?.value

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah would be a nice addition I think.

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 to me.

I have some comments/suggestions. Nothing blocking, if we want to get this merged, I think it's fine to leave them for a follow-up PR. Just please let's not forget them :)

@@ -97,7 +99,7 @@ type Pattern

Arguments:
- input: The text to match the pattern described by `self` against.
Copy link
Contributor

Choose a reason for hiding this comment

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

This could use a brief comment explaining the special handling of zero-length matches.

Remove dead argument from `select_columns`, `remove_columns` and `reorder_columns`.
Add ALIAS to `remove_columns`.
Add custom drop down for `keep_unmatched`.
Add fetch to URI.
Add some ALIASes (+, -, few methods).
@jdunkerley jdunkerley force-pushed the wip/jd/table-write-100 branch from d2518db to ad897c8 Compare June 1, 2023 20:26
@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Jun 1, 2023
@mergify mergify bot merged commit 343b5fb into develop Jun 1, 2023
@mergify mergify bot deleted the wip/jd/table-write-100 branch June 1, 2023 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Text.find_all can loop endlessly Integrate Execution Context with File Writing
3 participants