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

Port resize into a method outside of lily objects #51

Merged
merged 17 commits into from
Feb 21, 2021
Merged

Conversation

mrz1988
Copy link
Owner

@mrz1988 mrz1988 commented Feb 21, 2021

Summary

Remove all resize logic out of LilyBlock and LilyString classes, and create a new resize() helper method. The new resize method:

  • Is more opinionated on how to match colors
  • Uses new, better language (width, height, align)
  • Can take any sort of object, instead of requiring a cast first
  • Is smart about the type of object it returns
  • Is simpler and more obvious in its use cases

Technical Implications

  • LilyString no longer has a resize() method.
  • LilyBlocks no longer have resize methods, including resize_x and resize_y.
  • LilyBlock.normalize() is now replaced by resize(block).
  • grow() now returns a LilyString if an iterable of length 1 is added in (instead of a 1-row LilyBlock).

Why?

As the repo grows, it makes sense to keep lily text objects a bit thinner, and have powerful helper methods do most of the heavy lifting. This is advantageous for a few reasons:

  • Lily text objects can be thinner, keeping business logic elsewhere
  • The API is easier to learn and understand, with only a handful of useful methods
  • Internal logic on Lily text objects require less validation of user input, isolating that elsewhere
  • It gives us an opportunity to clean up and unify some of the more confusing aspects of the resize parameters/logic

@codecov
Copy link

codecov bot commented Feb 21, 2021

Codecov Report

Merging #51 (0e2b633) into master (7e800e6) will decrease coverage by 0.20%.
The diff coverage is 91.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
- Coverage   88.12%   87.92%   -0.21%     
==========================================
  Files          34       35       +1     
  Lines        1768     1813      +45     
  Branches      281      301      +20     
==========================================
+ Hits         1558     1594      +36     
- Misses        175      183       +8     
- Partials       35       36       +1     
Impacted Files Coverage Δ
lilies/style/styles.py 100.00% <ø> (ø)
lilies/objects/lilyblock.py 79.88% <76.92%> (-1.71%) ⬇️
lilies/api/resize.py 91.30% <91.30%> (ø)
lilies/__init__.py 81.81% <100.00%> (ø)
lilies/api/__init__.py 100.00% <100.00%> (ø)
lilies/api/columnify.py 96.72% <100.00%> (+0.05%) ⬆️
lilies/api/grow.py 100.00% <100.00%> (ø)
lilies/objects/lilystring.py 81.61% <100.00%> (-0.33%) ⬇️
lilies/style/__init__.py 100.00% <100.00%> (ø)
lilies/compiler/state_manager.py 82.22% <0.00%> (-4.45%) ⬇️
... and 3 more

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 7e800e6...0e2b633. Read the comment docs.

@mrz1988 mrz1988 merged commit b78ab6d into master Feb 21, 2021
@mrz1988 mrz1988 deleted the refactor-resize branch February 21, 2021 22:24
mrz1988 added a commit that referenced this pull request Feb 22, 2021
Summary
=======
Remove old styling by index and regex on LilyString and LilyBlock objects, and implement a new `highlight()` utility method that does a similar thing.

Technical Details
=============
Support has been removed for the following methods:

- LilyString.style_chars
- LilyString.style_char
- LilyString.style_regex
- LilyBlock.style_regex

Why?
====
This continues along the theme of #51 to reduce the footprint and responsibility of Lily object classes, and instead provide powerful, intuitive utility functions. Prefer universal tools rather than forcing casts.
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.

None yet

1 participant