1. 12

    There’s no reason to manually review those changes. If they pass your automated test suite and CI system, they should just be merged.

    I have my doubts about that…

    1. 5

      I’ve worked on a system that was well-tested enough for that to be a viable approach once (ever) in my career, and even then we never felt super comfortable about it.

      1. 2

        Do you have rationale reasons or just a feeling?

        1. 9

          People (unintentionally) write bad tests. Also, giving microsoft permission to commit literally anything to your repos leaves the door open for possible abuse in the future when, for example, they decide at some point they no longer ‘love’ open source.

          1. 2

            Can’t say for the OP, but I had an issue where all tests went green after a dependency version update, only to have everything start failing after merging the file. The problem was that the tests were not comprehensive enough, and there was a subtle difference between tests on master vs PR.

          2. 1

            The big issue is that with updates to dependencies come changed behavior that may be subtle enough your tests don’t catch it.

          1. 2

            This sort of “mini interpreter in an object/module” stuff is so cool. Really interested to see the next installment.

            One thing I found myself curious about was the change from the

            return lambda value: op(value, nodes)
            

            in the original implementation of the build_evaluator method to the explicit declaration (and return) of the _op function in the updated code. These two methods achieve the same goal, correct?

            1. 2

              They achieve the same goal, that might be just the change of form from lambda to a def that confuses you I imagine? Other than that, the idea is the same indeed.

            1. 5

              Seems to be some kind of viewport issue on iOS / Safari- text on the left and right is clipped off:

              https://imgur.com/a/kl9P6PG

              1. 4

                I have the same issue on mobile chrome.

                1. 1
                1. 4

                  I was expecting Python-AST wizardry and got disappointed. But not as much as by the fact that this is Python-2-style code. Otherwise it’s a thought-inspiring article that boils down to a “use a syntax tree built from nested dicts instead of writing your own DSL parser”.

                  Edit: technically correct is the best kind of correct

                  1. 2

                    What makes you say it’s Python 2?

                    1. 3

                      Usage of u"" and inheriting from object. Former was just added to Python 3.3+ for backwards compatibility; strs are unicode by default in Python 3 anyway. Making new-style classes doesn’t require inheriting from object in Python 3 (even in 2.7 I think) because old-style classes have been removed altogether.

                      Imo, code written today should not aim for Python 2 compatibility unless it’s something that has to work with a Python 2 code base that’s not migrated yet.

                      1. 3

                        Ok. I don’t think you should write “this is Python 2” when the code is (indeed) compatible with both Python 2 and 3.

                        1. 4

                          You’re right, technically it’s Python-3-compatible. It’s still Python-2-style code, which I think is bad because it encourages a soon-to-be-EOL’d Python version. Newly written code should just be Python-3-only.