search

bobjacobsen / python-openlcb

MIT License
2 stars 1 forks source link

Review FIXME comments #35

Closed Poikilos closed 2 months ago

Poikilos commented 3 months ago

More in FIXME and maybe more not? Tests pass, but try examples again. I'm not completely sure how to interpret the output (to determine whether it is as expected). At least one FIXME comment is added in #32, but the comments are regarding code that predates the PR.

bobjacobsen commented 3 months ago

37 fixes several of these. Not sure what to do about two of the remaining ones:

PhysicalLayer only contains three pass methods. It's there because of the migration from Swift. Perhaps the thing to do is to remove the test file entirely?

Doesn't seem to be a problem there. What am I missing?

Poikilos commented 3 months ago
  • ./tests/test_physicallayer.py:10: # FIXME: finish this

PhysicalLayer only contains three pass methods. It's there because of the migration from Swift. Perhaps the thing to do is to remove the test file entirely?

Maybe keep it as a placeholder so we can easily add a test in case we figure out a way to test parts of the submodule without hardware attached. If it doesn't seem necessary now, maybe just change FIXME: finish this to TODO: implement offline tests if relevant in cases where tests only have "pass"

Poikilos commented 3 months ago
  • ./openlcb/memoryservice.py:301: # FIXME: Python resizes ints automatically based on value.

Doesn't seem to be a problem there. What am I missing?

If it doesn't cause a problem, maybe just change:

        Returns:
            int: The converted data as a number.
        """
        # FIXME: Python resizes ints automatically based on value.

to a clarification (since return size may contradict method name arrayToUInt64, but not cause a problem):

        Returns:
            int: The converted data as a number (Python determines
                actual in-memory size based on the value).
        """
bobjacobsen commented 3 months ago

Thanks for showing how you wanted those fixed. Added to #37.

Poikilos commented 2 months ago

I searched for "FIXME" again and I think everything is covered if you make that change to the comments. Ready to merge #37 after that?

Poikilos commented 2 months ago

I went ahead and made the changes we discussed here, so to get those you can review and pull #39 which merges into the fixme_once branch, before merging #37 if you see #39 is acceptable.

bobjacobsen commented 2 months ago

Oops. I merged #37 before merging your #39, which closed #39. Will fix by created a new PR that merges your branch to main. Sorry for my confusion.