added base64

[zacharias1219]

Issue number #158

[richard-to]

This is a good start. Let's add some test coverage now.

You'll need tests for:

• mesop/dataclass_utils/dataclass_utils_test.py
• mesop/dataclass_utils/diff_state_test.py
• mesop/web/src/utils/diff_state_spec.ts

To run the specific tests:

bazel test //mesop/dataclass_utils:dataclass_utils_test
bazel test //mesop/dataclass_utils:diff_state_test
bazel test //mesop/web/src/utils:unit_test

[zacharias1219]

Hey I've added tests and removed the period

[richard-to]

For diff_test_spec.ts, can you add an example for bytes (there's a lot of formatting changes, so it's possible I missed the test case there)?

Also for dataclass_utils_test, can you add a couple more examples for test_serialize_bytes(), such as empty string and maybe something with unicode characters. For those tests you can use pytest parametrized tests: https://docs.pytest.org/en/7.1.x/how-to/parametrize.html#pytest-mark-parametrize-parametrizing-test-functions

[richard-to]

Also in your PR description, can you reference the issue number?

[zacharias1219]

Hey I've made changes as requested.

And extremely sorry about the formatting change, whenever i saved the file it suddenly showed all blue changed lines and I didn't know why, thought there was an error with my editor.

[richard-to]

Thanks for making those changes. This looks good. I added a few minor comments about removing unnecessary comments. But after I can approve and merge this PR in for you. Thanks!

[richard-to]

Looks like the linting presubmit failed. Can you adjust based on the errors there. You can also install the precommit checks for checking. See https://google.github.io/mesop/internal/development/

[zacharias1219]

Yes i'll look in it.

[zacharias1219]

I ran pre-commit on the entire file and it has modified most of the files.

[zacharias1219]

I'm not sure if the changes that I made are correct, i just ran pre-commit and it made some changes of it's own.

[richard-to]

Yes, part of the precommit steps will run formatting checks to make sure the code is formatted consistently in our codebase.

Can you run pre-commit run --all-files. This should update the formatting for: mesop/web/src/utils/diff_state_spec.ts

Example of the changes it needs to make for be consistent:

[zacharias1219]

Okay.
When I did that, it changed all the files, should I commit all the files or just mesop/web/src/utils/diff_state_spec.ts

[richard-to]

Oh yeah, you should just include mesop/web/src/utils/diff_state_spec.ts. I'm running the pre-submit again. 🤞🏾

[zacharias1219]

Ahh man thought it would be done.

[zacharias1219]

Hold on it changed the files again when I ran pre-commit run --all-files

[zacharias1219]

There are some changes in .pre-commit-config.yml as well, should i keep those changes??

[richard-to]

Leave the .pre-commit-config.yaml as is for now. Just upload the change to: mesop/dataclass_utils/dataclass_utils.py

I think that should be it.

[zacharias1219]

Hey I done the changes, hopefully it's the last one.
I'll keep this step in check before doing pr's next time, sorry for all the headache.

[richard-to]

Hey I done the changes, hopefully it's the last one.
I'll keep this step in check before doing pr's next time, sorry for all the headache.

It's not a problem at all. I think this just tells us that we need to improve our documentation for setting up the dev environment better. That's usually the most annoying part which is partially why we've been working on setting up Docker Images and Github Codespaces.

[zacharias1219]

I genuinely don't know why it's going wrong

[zacharias1219]

Okay I made changes, however it's showing failed for these two, don't know what to do here, could you help.

[richard-to]

Somehow the indention got misaligned here. That's what the error messages are saying:

[zacharias1219]

Yes this is what i've change just now, however there are other two errors that I'm getting.

[zacharias1219]

Okay this run seems promising.

[richard-to]

Looks like the next thing to fix is the unit tests.

For this test, "test_diff_bytes_fails" you can delete it now that we have support for bytes.

To run the tests locally:

bazel test //mesop/dataclass_utils:dataclass_utils_test

[zacharias1219]

Okay 1 test reduced at least. I'll look into the other one.

[zacharias1219]

Hey, Hopefully this works, I'll be going to sleep right now, it's really late here, will work on it tomorrow if it fails.

[zacharias1219]

What version of bazel do you recommend for this?
Because the checks that I'm doing on my local machine are passing but not after you start the checking process.

[richard-to]

What version of bazel do you recommend for this? Because the checks that I'm doing on my local machine are passing but not after you start the checking process.

Looking at the error, do you think there is something non-deterministic with the base64 encoding or something? Seems like you get a different encoding your local machine than on the CI machine.

I think for now you can just remove unicode test.

[zacharias1219]

Is this it, I'm not sure at this point.

[zacharias1219]

The pre checks were all fine for this and checked the bazel test, no errors hopefully this is the last one.

[richard-to]

Ok, looks like it passed. Thanks for you patience on this @zacharias1219.

[zacharias1219]

Ohh thank god, finally.

[zacharias1219]

Is there anything I should do more, and yeah I should be thanking you for being patient.

[richard-to]

I think that's it for this issue.

…

On Thu, Jun 13, 2024 at 7:35 AM Richard Abishai ***@***.***> wrote: Is there anything I should do more, and yeah I should be thanking you for being patient. — Reply to this email directly, view it on GitHub <#415 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEDZ4I3KEMLPF5DKSWZC4LZHGU27AVCNFSM6AAAAABJDRGVZOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRVHA3DAMRQGQ> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[zacharias1219]

Thank you so much for being a good mentor during this pr. I'll look forward to contributing more to this project.