Values::FunctionalGroupVal#at missing?

[natec425]

https://github.com/irobayna/stupidedi/blob/a0f3529892da67ce42ba389e25f3a9c2c7494b91/lib/stupidedi/values/functional_group_val.rb#L41-L67

Hi hi,

Sorry if I'm a nuisance with these issues!

It seems like at is missing in these methods. I went history diving, and it seems like SegmentValGroup#at was removed in this commit: https://github.com/irobayna/stupidedi/commit/2df3799b4f2d0585813dc88cb90128526298cc24#diff-1840014d69711790cc284d9a35332a5b. If I understand correctly, this would (maybe accidentally) remove it from FunctionalGroupVal.

Please let me know if I'm misunderstanding something. If I'm correct that it is missing, I'd be happy to take a swing at a PR.

Thanks for your time!

[kputnam]

Not a nuisance at all, thank you for doing this. That change was in the early days so I can't remember my thought process, but I think that's when I moved away from trying to address segments by using their position (the nth child of some known parent) and figured out the #find(:GS) interface.

My vote here is to remove the #version, #release, #subrelease, and #implementation methods. We'd need to be certain they aren't used somewhere, but I think that stuff is parsed in FunctionalGroupState.push.

If it's needed by the user, it's stored in the parse tree but we might need to write a little code to make it easier to access. But FunctionalGroupState has #gs08 and #gs01 attributes and we could add #implementation, #version etc, if it's wanted. For now, you can access that with something along the lines of find(:GS).tap{|gs| gs.active.head.up.gs08). But I'd guess people that need those values are already doing something like gs08 = find(:GS).flatmap{|e|e.elementn(8) and parsing those values themselves.

Please keep these issues coming and submit PRs. Since they are code quality issues I would like to get them fixed. I realize reviewing a ton of code you didn't write can be a lot of work, so thank you!

[natec425]

I'm glad to hear that my work is welcome.

Would you be interested in a PR trying out sorbet on some Values classes?

[kputnam]

I'm not familiar with it but I'll take a look. I will be offline for a few weeks, so I'll look when I'm back. Thanks Nate!

[kputnam]

Hey Nate,

I looked briefly at sorbet and it looks like it could be very helpful. I like the philosophy, that it's not tied to a particular editor, and that performs static analysis. Changing @x = expr to @x = T.let(expr, type) seems a little ugly, but that's not a deal-breaker. The method signatures wouldn't interfere with the existing code and are better than using a YARD comment.

One issue I'd want to know about before moving forward is support for parameterized types. There are a lot of these in the stupidedi code base, like AbstractCursor<Int> means AbstractCursor#node would return Int. It looks like there must be some support because they have examples that have syntax like T::Array[Int], but I haven't found the docs on it yet.

The second issue is that I would like to have some kind of stub for T::Sig so if we decide to use sorbet only during development and testing, users won't need to install it as a dependency. It might be overly cautious on my part, so we may later decide not to use the stub. But I want to be sure there aren't adverse performance effects or bugs that are introduced or some kind of farpotshket situation.

Feel free to submit a PR that starts work on this, I'd be very interested. I have a lot of unmerged work in gh-65 that I think renames some modules, so it would save me some pain if you started from that branch. I believe it's stable and builds without errors. I'm currently working on C extension code in a separate branch called gh-65.3, so we wouldn't be stepping on each others toes.