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

Refactored enso-data crate and text utilities. #3166

Merged
merged 20 commits into from
Nov 25, 2021

Conversation

farmaazon
Copy link
Contributor

@farmaazon farmaazon commented Nov 23, 2021

Pull Request Description

This is implementation of task https://www.pivotaltracker.com/n/projects/2539304/stories/180312587. The old enso-data crate was renamed to enso-data-structures and designed to have only those: data structures.

That required moving the structures related to Text positions (the text module containing Index and Span). Because the EnsoGL text area component has structures duplicating functionalities, they were moved to a separate enso-text crate and replaced the old usages of Index and Span.

Important Notes

  • This refactoring required some thinking, what offsets should be primarily used in our code. I tried to have a consistent outcome without much more rewriting (the rewriting can be a part of https://www.pivotaltracker.com/n/projects/2540121/epics/4864521).
  • As a side effect, the old issues with non-ascii characters in nodes are fixed.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the Scala, Java, and Rust style guides.
  • All documentation and configuration conforms to the markdown and YAML style guides.
  • All code has been tested where possible.

@farmaazon farmaazon self-assigned this Nov 23, 2021
@farmaazon farmaazon marked this pull request as ready for review November 23, 2021 15:06
Copy link
Contributor

@MichaelMauderer MichaelMauderer left a comment

Choose a reason for hiding this comment

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

Should we do some more testing based on a list like the Big List of Naughty Strings? That could make sure we didn't miss any corner cases.

app/gui/language/ast/impl/src/id_map.rs Show resolved Hide resolved
app/gui/language/ast/impl/src/crumbs.rs Outdated Show resolved Hide resolved
app/gui/language/ast/impl/src/lib.rs Outdated Show resolved Hide resolved
@@ -873,6 +873,16 @@ impl AreaModel {
_ => None,
}
}

/// Constrain the selection to values valid inside of the current text buffer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be useful to explain valid according to what rules.

lib/rust/text/src/text.rs Outdated Show resolved Hide resolved
@farmaazon
Copy link
Contributor Author

farmaazon commented Nov 24, 2021

Should we do some more testing based on a list like the Big List of Naughty Strings? That could make sure we didn't miss any corner cases.

Well, I'm sure that we won't work properly on those naughty strings, because this task resolves only one sort of problems (others are covered by https://www.pivotaltracker.com/n/projects/2539304/stories/180392693)

Copy link
Contributor

@mwu-tow mwu-tow left a comment

Choose a reason for hiding this comment

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

Not blocker, just for consideration: I'd rather see explicit unit::Bytes to avoid confusion with bytes::Bytes, also singular Byte might make more sense than Bytes

@farmaazon
Copy link
Contributor Author

Not blocker, just for consideration: I'd rather see explicit unit::Bytes to avoid confusion with bytes::Bytes, also singular Byte might make more sense than Bytes

If it is not a hard blocker, I would skip it for a while. I tried to apply this, but some things does not look well: Column and Line are also units, but this code:

pub fn first_line_start_column(&self) -> unit::Column

looks strangely to me: Without context, I would not know what unit is.

@farmaazon farmaazon merged commit 9ab4f45 into develop Nov 25, 2021
@farmaazon farmaazon deleted the wip/ao/refactoring-data branch November 25, 2021 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants