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

Bylabel exact #1459

Merged
merged 14 commits into from Dec 27, 2017

Conversation

@pythonissam
Copy link
Contributor

commented Dec 2, 2017

Before submitting your PR, check that you've:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)
@pythonissam

This comment has been minimized.

Copy link
Contributor Author

commented Dec 2, 2017

Issue: #1454

I have no confidence if my English is right. Please correct any sentences you feel unconfortable.
Thank you.

@pythonissam

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2017

Well, fileByLabel also uses T.isInfixOf. Should I create the exact version for this, too?

@snoyberg

This comment has been minimized.

Copy link
Member

commented Dec 3, 2017

nameFromLabel :: T.Text -> RequestBuilder site T.Text
nameFromLabel label = do
--
-- @since 1.5.9

This comment has been minimized.

Copy link
@MaxGabriel

MaxGabriel Dec 3, 2017

Member

You don't need to give @since annotations for non-exported functions

Edit: unless this is meant to be exported?

This comment has been minimized.

Copy link
@pythonissam

pythonissam Dec 4, 2017

Author Contributor

No. I understand the rule. I'll do so:)

@@ -566,6 +570,16 @@ nameFromLabel label = do
(<>) :: T.Text -> T.Text -> T.Text
(<>) = T.append

-- |
-- @since 1.5.9
byLabelWithMatch :: (T.Text -> T.Text -> Bool) -- ^ The matching method which is used to find labels (i.e. exact, contains)

This comment has been minimized.

Copy link
@MaxGabriel

MaxGabriel Dec 3, 2017

Member

Is this meant to be exported? It seems really useful

This comment has been minimized.

Copy link
@pythonissam

pythonissam Dec 4, 2017

Author Contributor

No, neither.

-- > byLabel "Nickname" "Snoyberger"
--
-- Therefore, this function is deprecated. Please consider using 'byLabelExact',
-- which performs the exact match over the given word.

This comment has been minimized.

Copy link
@MaxGabriel

MaxGabriel Dec 3, 2017

Member

I think we should use a GHC DEPRECATED pragma, and recommend byLabelWithMatch Data.Text.contains for the old behavior and byLabelExact for the new behavior

This comment has been minimized.

Copy link
@pythonissam

pythonissam Dec 4, 2017

Author Contributor

recommend byLabelWithMatch Data.Text.contains for the old behavior and byLabelExact for the new behavior

Actually, byLabelWithMatch is the abstraction of the matching methods and byLabel performs the old behavior (contain) as before, unless users will be forced to rewrite their tests to use byLabelContain or whatever. And yes, byLabelExact is for the new behavior.

So,

byLabel -> deprecated
byLabelExact -> recommanded

Is this okay?

I think we should use a GHC DEPRECATED pragma

NiceXD. I even didn't know that. I'll modify the code.

@@ -591,12 +605,57 @@ nameFromLabel label = do
-- > <form method="POST">
-- > <label>Username <input name="f1"> </label>
-- > </form>
--
-- Note that this function finds the labels in which contain the given word.
-- It might choice labels unexpectedly or just fail in the circumstances like below:

This comment has been minimized.

Copy link
@MaxGabriel

MaxGabriel Dec 3, 2017

Member

Can you make this more explicit about how the function would fail? Something like the following:

Warning: This function looks for any label that contains a subset of the provided text. If multiple labels contain that subset, this function will throw an error, as in the example below:

Site note since it sounds like you're working on your English skills:

"labels in which contain" should be "labels which contain"

"It might choice labels" should be "It might choose labels"

This comment has been minimized.

Copy link
@pythonissam

pythonissam Dec 4, 2017

Author Contributor

Can you make this more explicit about how the function would fail?

Sure.

And thank you for your advice about my English. It helps me a lot.

@pythonissam

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2017

@snoyberg Should I create another PR?

@snoyberg

This comment has been minimized.

Copy link
Member

commented Dec 4, 2017

I don't think a new PR is necessary, I'd say just update this one.

kotaro
@pythonissam

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2017

@MaxGabriel Following your advices, I updated haddocks. What do you think?

@@ -620,12 +679,46 @@ byLabel label value = do
-- > <form method="POST">
-- > <label>Please submit an image <input type="file" name="f1"> </label>
-- > </form>
--
-- Warning: There exists the same issue of 'byLabel'. Please use 'fileByLabelExact' instead.

This comment has been minimized.

Copy link
@MaxGabriel

MaxGabriel Dec 17, 2017

Member

It would sound better to say "This function has the same issue as 'byLabel'"

@MaxGabriel

This comment has been minimized.

Copy link
Member

commented Dec 17, 2017

I made one small comment. Otherwise LGTM!

kotaro
@pythonissam

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2017

@MaxGabriel Noted with thanks!

@pythonissam

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2017

@snoyberg @MaxGabriel I guess I've done. Would you review changes?

@MaxGabriel

This comment has been minimized.

Copy link
Member

commented Dec 18, 2017

@pythonissam what’s the other bug in byLabel?

@pythonissam

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2017

@MaxGabriel Please consider the following situation

<html>
  <form>
    <label>foo2<input type=text name="bar2"></label>
    <label>foo <input type=text name="bar" ></label>
  </form>
</html>

Then byLabel "foo" "baz" sets baz into bar2, not bar.

The cause is around here:

-- In genericNameFromLabel

case mfor of
  for:[] -> ...

  [] ->
    case filter (/= "") $ mlabel >>= (child >=> C.element "input" >=> attribute "name") of
      [] -> failure $ "No label contained: " <> label
      name:_ -> return name

  _ -> ...

Using isInfixOf, mlabel will be ["foo2","foo"]. The above form doesn't contain any for attributes, so mfor will be an empty list.

Finally, filter (/= "") $ mlabel >>= (child >=> C.element "input" >=> attribute "name") becomes ["bar2", "bar"] and its head is taken.

@pythonissam

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2017

Same here, the fundamental cause is that we decided to use isInfixOf. So I feel like there's nothing we can do without changing their behaviors.

@pythonissam

This comment has been minimized.

Copy link
Contributor Author

commented Dec 24, 2017

@MaxGabriel @snoyberg

Hi. What's the status of this? CI looks like it failed.
Is there anything I can do with this?

@snoyberg
Copy link
Member

left a comment

It looks good to me, I just had a minor comment on the deprecation notices. Thanks!

@@ -162,6 +164,8 @@ import GHC.Exts (Constraint)
type HasCallStack = (() :: Constraint)
#endif

{-# DEPRECATED byLabel "This function seems to have multiple bugs. Use byLabelExact instead" #-}

This comment has been minimized.

Copy link
@snoyberg

snoyberg Dec 24, 2017

Member

How about including a link to this PR in the deprecation notices?

This comment has been minimized.

Copy link
@pythonissam

pythonissam Dec 27, 2017

Author Contributor

It sounds good

@pythonissam

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2017

@snoyberg Is this okay?

@snoyberg snoyberg merged commit 3df8260 into yesodweb:master Dec 27, 2017

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
@snoyberg

This comment has been minimized.

Copy link
Member

commented Dec 27, 2017

Looks great, thanks!

@MaxGabriel

This comment has been minimized.

Copy link
Member

commented Dec 27, 2017

Thanks @pyhtonissam!

@pythonissam

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2017

@snoyberg @MaxGabriel Yay! Thank you, too!

MaxGabriel added a commit that referenced this pull request Dec 30, 2017
MaxGabriel added a commit that referenced this pull request Dec 30, 2017
MaxGabriel added a commit that referenced this pull request Dec 30, 2017
snoyberg added a commit that referenced this pull request Dec 30, 2017
Merge pull request #1473 from yesodweb/fix1459
Fix Haddock syntax error and test failures introduced by #1459
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.