search

LaurensWeyn / Spark-Reader

A tool to assist non-naitive speakers in reading Japanese
GNU General Public License v3.0
30 stars 7 forks source link

Question: old deconjugator #5

Closed wareya closed 7 years ago

wareya commented 7 years ago

I re-added the old deconjugator, which was straightforward enough.

Then I went to put back the same validity tests the old deconjugator had. Because they're done outside the old deconjugator and reach into StdRule and ValidWord (I purged impliedTag in the new deconjugator, etc), I had to make a lot of changes, and it's honestly kind of nasty; the amount of code is just ridiculous and I had to add unsafe casts.

https://github.com/wareya/Spark-Reader/commit/4db4b9c889b8b72c1c73e536273d8ef6db9f4fb4

It's up to you if you want me to make the new deconjugator PR with or without the original validity tests. It seems to work fine with the new one, so my vote is for without the original validity tests since they'd just make it harder to maintain.

Here's the commit that re-adds the old deconjugator (without the old validity tests): https://github.com/wareya/Spark-Reader/commit/ff106ecbf39ac023be77a3e74b5065f213a43c6d

LaurensWeyn commented 7 years ago

If the old deconjugator works with your updated ValidWord/StdRule classes, then there should be no reason to add a duplicate that does the same thing. Perhaps the parts of the code you didn't change could be left in a 'new' old StdRule and you can extend and override that to add your extra features, but that's not really important if it isn't needed.

I tend to make code that works first, then redo it later for speed and neatness when needed, so I'm fine with it being a bit of a mess at first.

I'm planning to release 0.6 soon so I can get some feedback on the new basic Epwing support. After that, when you're ready, you can add your changes to the main branch. I think I can add you to the main repository as well so you can make improvements after it's been added, I'll look into trying that.

wareya commented 7 years ago

Yeah, sounds like the new tests are good then.

I don't badly need commit access even once the new deconjugator is added, PRs are how I'm used to doing things even with a game I worked on for a while, but we'll see how it goes.