Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes and updates for use in our Languages API #10

Merged
merged 8 commits into from
Jul 28, 2020
Merged

Conversation

pbrisbin
Copy link
Member

  • 4e7b147 Updates for newer GHC and --pedantic

    • Remove unused import
    • Use DerivingStrategies throughout
  • 414f9ab Fix style in touched files

  • 3e69807 Add HttpApiData orphans

    To use BCP47 as a Persist key, we need these instances.

  • 21d5f9e Export parser

    It can be useful to parse BCP47 tags in the context of a larger parser, such
    as parsing the HTTP Accept-Language Header, which is formatted as

    {language}[;q={weight}][,...]
    

    Doing this on top of fromText is possible, but a bit annoying.

    NOTE: I attempted to move this to a Data.BCP47.Parser, but the current
    top-level module contains too many lower-level data definitions for that to be
    possible without an import-cycle. To address that, we could move most of
    Data.BCP47 to .Internal modules themselves, and leave Data.BCP47 as a
    re-export, higher-level interface for the most common usage. This would be a
    Good Thing, IMO, but has been deferred for now.

- Remove unused import
- Use DerivingStrategies throughout
To use BCP47 as a Persist key, we need these instances.
It can be useful to parse BCP47 tags in the context of a larger parser,
such as parsing the HTTP Accept-Language Header, which is formatted as

    {language}[;q={weight}][,...]

Doing this on top of fromText is possible, but a bit annoying.

NOTE: I attempted to move this to a Data.BCP47.Parser, but the current
top-level module contains too many lower-level data definitions for that
to be possible without an import-cycle. To address that, we could move
most of Data.BCP47 to .Internal modules themselves, and leave Data.BCP47
as a re-export, higher-level interface for the most common usage. This
would be a Good Thing, IMO, but has been deferred for now.
@pbrisbin pbrisbin requested a review from eborden July 27, 2020 15:27
Copy link
Contributor

@eborden eborden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM

@pbrisbin
Copy link
Member Author

Really not sure what's up on CI:

Progress 1/2: bcp47�������������������                   �������������������bcp47        > 
Progress 1/2: bcp47�������������������                   �������������������bcp47        > library/Data/BCP47/Internal/Parser.hs:9:1: error:
Progress 1/2: bcp47�������������������                   �������������������bcp47        >     Could not load module ‘Text.Megaparsec’
Progress 1/2: bcp47�������������������                   �������������������bcp47        >     It is a member of the hidden package ‘megaparsec-7.0.5’.
Progress 1/2: bcp47�������������������                   �������������������bcp47        >     You can run ‘:set -package megaparsec’ to expose it.
Progress 1/2: bcp47�������������������                   �������������������bcp47        >     (Note: this unloads all the modules in the current scope.)
Progress 1/2: bcp47�������������������                   �������������������bcp47        >     Use -v to see a list of the files searched for.
Progress 1/2: bcp47�������������������                   �������������������bcp47        >   |
Progress 1/2: bcp47�������������������                   �������������������bcp47        > 9 | import Text.Megaparsec (Parsec, eof, lookAhead)
Progress 1/2: bcp47�������������������                   �������������������bcp47        >   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Progress 1/2: bcp47�������������������                   �������������������bcp47        > 
Progress 1/2: bcp47�������������������                   �������������������bcp47        > library/Data/BCP47/Internal/Parser.hs:10:1: error:
Progress 1/2: bcp47�������������������                   �������������������bcp47        >     Could not load module ‘Text.Megaparsec.Char’
Progress 1/2: bcp47�������������������                   �������������������bcp47        >     It is a member of the hidden package ‘megaparsec-7.0.5’.
Progress 1/2: bcp47�������������������                   �������������������bcp47        >     You can run ‘:set -package megaparsec’ to expose it.
Progress 1/2: bcp47�������������������                   �������������������bcp47        >     (Note: this unloads all the modules in the current scope.)
Progress 1/2: bcp47�������������������                   �������������������bcp47        >     Use -v to see a list of the files searched for.
Progress 1/2: bcp47�������������������                   �������������������bcp47        >    |
Progress 1/2: bcp47�������������������                   �������������������bcp47        > 10 | import Text.Megaparsec.Char (char)
Progress 1/2: bcp47�������������������                   �������������������bcp47        >    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Progress 1/2: bcp47�������������������                   �������������������bcp47        > Test suite doctest failed

I can't reproduce it locally.

@eborden
Copy link
Contributor

eborden commented Jul 27, 2020

Old cache?

@pbrisbin
Copy link
Member Author

I can try busting it, but it doesn't appear to be that (everything seems to build that I'd expect to build). So I'm afraid if busting it works I might've just papered over something that'll bite someone else later.

@pbrisbin
Copy link
Member Author

pbrisbin commented Jul 28, 2020

It seems to be this bug: commercialhaskell/stack#5159 "Doctests in multi-package project interfering with each other"

I can repro locally (only) with:

stack build -j1 bcp47:doctest bcp47-orphans --stack-yaml "stack.yaml" --no-terminal  --pedantic --test                                       

EDIT: This still doesn't reproduce reliably. Locally it almost always works, and on CI it almost always fails.

We were incorporating eof into the parser in two places: parser, and
complete. These were preventing parser from being usable in the context
of something larger (see _example) because the eofs were expected.

This required two fixes:

1. Update complete to also end at non-tag characters (not just eof)
2. Move the eof from parser out to fromText

   fromText is intended to be used on a complete value, so looking for
   eof here is required. When/if parser is used in the context of a
   larger parser it can not look for eof.
@pbrisbin
Copy link
Member Author

OK, building the packages independently seems to work around the shared-environment bug. I've cleaned up history (wiping out other failed workarounds), and will merge on Green.

@pbrisbin pbrisbin merged commit f554d49 into master Jul 28, 2020
@pbrisbin pbrisbin deleted the pb/updates branch July 28, 2020 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants