Canceled.
Le lun. 13 juil. 2026 à 18:17, Earl Chase <[email protected]> a écrit :
>
> I'm just going to cancel this thread. It seems like you want to go in
> a different direction, which is fine. I'm not a reviewer so I don't
> see what the point of reviewing your code would be.
>
> Le lun. 13 juil. 2026 à 18:10, Morgan Smith
> <[email protected]> a écrit :
> >
> > Hello Earl!
> >
> > I apologize for the delay. Your most recent patch has now sat unreviewed
> > for
> > almost a month. It was not my intention to let it sit that long.
> >
> > I also apologize for changing my mind a few times in this thread. This is
> > my
> > first proper review and I am a bit of perfectionist. We have had more back
> > and
> > forth then is probably necessary due to my lack of experience.
> >
> > I have thought about this for a while and I have decided that presenting a
> > refined patch in response to someone's patch is perfectly reasonable (and
> > just
> > FYI is something I've had reviewers do to me on many occasions). However, I
> > don't believe the language I used when presenting the patch in this thread
> > was
> > ideal. It did indeed sound like I was "taking over" this issue. My
> > intention
> > was to collaborate and thus in the future I will try to use language that
> > emphasizes that intention.
> >
> > I do stand by a pretty important idea that I have been trying to
> > communicate in
> > this thread: do 1 clear change per commit. You're still trying to do both
> > the
> > API change over and reorganization in 1 commit.
> >
> > Moving some of the functionality of the monolithic function into
> > multiple, smaller, well documented functions could make the code more
> > readable. However, I'm not convinced your reorganization accomplishes
> > this. As I've said previously (although probably not communicated the
> > best), the reorganization can wait until a future thread where we take
> > advantage of some reorganization to implement some new features.
> >
> > To repeat: I do want the code reorganized. I just want to do that at a
> > later
> > time when we're implementing new features.
> >
> > See attached two patches.
> >
> > The first one is the new tests you've submitted. I refactored them a little
> > but they are mostly untouched. They are good tests! Thank you very much
> > for
> > creating them!
> >
> > > I added week, month and years tests
> >
> > I don't see any tests for year repeaters. Not strictly necessary but it
> > would
> > be nice to have some.
> >
> > > In the future, it would be good to actually ... assert the correct errors
> > > are
> > > thrown for each type of malformed data.
> >
> > ERT has ways to check for the type of error (ex. calc does `(signal
> > 'math-overflow nil)`) but it does not have a way of checking the error
> > message
> > as far as I know. I would also be interested in testing errors more
> > precisely.
> >
> > I've split the tests out into its own commit so we can run these new tests
> > before and after our changes are applied.
> >
> > The second commit is I believe actually identical to the first patch I sent
> > you. Using the following git command you can see the logic and ordering is
> > the
> > same while only switching out the API
> >
> > git show --word-diff=color --word-diff-regex='[^[:space:]()]+'
> > --diff-algorithm=histogram HEAD
> >
> > If you're happy with those two commits, then I can apply them and we can
> > close
> > out this thread. The commits have already passed my fairly comprehensive
> > local
> > CI system.
> >
> > Tests pass after each patch on emacs 30.2.
> > Tests pass after final patch on emacs 28 and 29 and a recent emacs/master
> > build.
> > Tests pass after final patch with TZ set to UTC, Europe/Istanbul, and
> > America/New_York.
> > Tests pass after final patch with the locale set to C, en_US, fr_FR, de_DE,
> > and
> > he_IL.
> >
> > > Is there any reason specifically that I should use `pcase' over `cl-case'?
> >
> > It was just a nitpick. cl-case is a simpler function that does exactly
> > what you where trying to do. Logically the solutions are identical. I
> > would imagine cl-case is faster but I haven't tested it.
> >
> >
> > I really do appreciate the work you've put into this. Overall you've
> > submitted
> > some really high quality patches! I do hope we can continue to work
> > together
> > on some things.
> >
> > I'm sorry this has dragged on so long and I appreciate your patience as I
> > learn
> > some communication skills and develop my standards for contributions.
> > You've
> > helped me in many ways in this thread :)
> >
