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

Installability and Compatibility #1

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

merlinfuchs
Copy link

I added a basic setup.py to make the package installable over git and also replaced the removesuffix method because it's only available in Python 3.9.
Ordinary dicts are only guaranteed to be ordered in Python 3.6+. Using them could potentially break the encoding for older Python versions.

It's also questionable if this iteration through the whole string is necessary as we could also check the validity in the iteration later:

    if not all(c in CHARACTER_VALUES.values() for c in text.replace(SECTION_SEPERATOR, '')):
        raise TypeError(f'Invalid bottom text: {text}')

I also don't see a reason to create a copy of the whole character dict here:

rev_mapping = {v: k for k, v in CHARACTER_VALUES.items()}

I didn't want to put these two parts into the Pull Request as I don't know if there are reasons for them.

@romdotdog
Copy link

romdotdog commented Feb 9, 2021

I also don't see a reason to create a copy of the whole character dict here

That is a reverse dictionary. I agree that it shouldn't be created every time from_bottom is called.

@romdotdog
Copy link

romdotdog commented Feb 9, 2021

Correct me if I'm wrong, couldn't you use rstrip? Even though it might not be valid bottom to not have a byte separator at the end, rstrip will grant leniency and a non-cryptic error message.

>>> "abcd".rstrip("cd")
'ab'

@merlinfuchs
Copy link
Author

I also don't see a reason to create a copy of the whole character dict here

That is a reverse dictionary. I agree that it shouldn't be created every time from_bottom is called.

Oh, you are right. Probably better to create that outside of the function.

Correct me if I'm wrong, couldn't you use rstrip?

>>> "abcd".rstrip("cd")
'ab'

rstrip does not remove the whole string from the end but each character separately. So "abcdd".rstrip("cd") would also result in "ab". It would still work as the two byte-separator characters only appear once anyways. I would imagine using [:-len(CHARACTER_VALUES)] is way more efficient tho.

@romdotdog
Copy link

I knew about the rstrip thing. If you're concerned about efficiency, then you should do [:-2]. My concern is that bottom-py is now more strict than before since you don't check whether the ending two characters are indeed the byte separator.

@merlinfuchs
Copy link
Author

merlinfuchs commented Feb 9, 2021

I did len(CHARACTER_VALUES) to have all spec-values at one place.
The spec clearly states that every bottomified string has to end with the byte-separators:
bottom -> values (BYTE_SEPARATOR values)* BYTE_SEPARATOR

Unless I'm misunderstanding it.

@romdotdog
Copy link

I guess this is a curse of the spec again on whether non-trailing byte separators should be disallowed.
bottom-rs allows no trailing byte separator:

https://github.com/bottom-software-foundation/bottom-rs/blob/710a61059272f5453c4ef37096126e45451e7fcf/src/bottom.rs#L46

@merlinfuchs
Copy link
Author

Yeah, kinda weird to require trailing separators tbh. I just followed the spec here, but I guess I can add if text.startswith(BYTE_SEPARATOR)

@romdotdog
Copy link

but I guess I can add if text.startswith text.endswith(BYTE_SEPARATOR)

Just FYI: the spec as of yet is not a reliable source as it has plenty of ambiguity not yet sorted out.

@merlinfuchs
Copy link
Author

Actually just noticed that reversing the dict is completely unnecessary as we never need the original dict. (value -> emoji)

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.

2 participants