unison icon indicating copy to clipboard operation
unison copied to clipboard

Add Explicit Export Lists for Modules that involve Parsing

Open setupminimal opened this issue 3 years ago • 3 comments

Overview

I wanted to start working on #499, but it wasn't entirely clear to me how the parser was structured, and therefore how it would be best to abstract it out. I figured that a good place to start would be adding explicit export lists, so that I could be sure of what functions were or were not used elsewhere.

This pull request adds those explicit export lists, which also revealed some top-level items which are not used, so I removed those as well. These two changes are split into two commits, so you can selectively merge only the first one if those things were needed for some reason.

Implementation notes

This just adds explicit export lists. No functional code change.

Interesting/controversial decisions

I don't think anything in this change should be controversial -- this is a small refactoring to prepare for deeper work on #499.

Test coverage

Since there is no new functionality or changes in functionality, I think existing tests are probably sufficient.

Loose ends

Doing this showed me that the parser is somewhat messily structured, and that there are no real private modules. I think a good next step is probably to pull out private modules (which would go in something like Unison.Parser.Default) for the things which aren't used outside parser-typechecker, and then hopefully the remaining public interface will be small enough to put into a typeclass or similar.


This change is Reviewable

setupminimal avatar Nov 23 '21 14:11 setupminimal

Hi @setupminimal, sorry for the delay — could you review and add yourself to the CONTRIBUTORS.md file as part of this PR, and I'll get to reviewing it :)

aryairani avatar Dec 23 '21 04:12 aryairani

Sorry about the delay on my end as well -- I somehow did not see your message. I've merged up, and hopefully CI will pass. If not, I'll fix it this evening.

setupminimal avatar Mar 06 '22 20:03 setupminimal

@setupminimal It looks like there are a couple of compiler warnings to fix up. Thanks!

aryairani avatar Mar 06 '22 23:03 aryairani