Bylabel exact by pythonissam · Pull Request #1459 · yesodweb/yesod

pythonissam · 2017-12-02T10:01:55Z

Before submitting your PR, check that you've:

Bumped the version number
Documented new APIs with Haddock markup
Added @since declarations to the Haddock

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 · 2017-12-02T10:05:58Z

Issue: #1454

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

pythonissam · 2017-12-03T09:00:24Z

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

snoyberg · 2017-12-03T16:24:50Z

Sounds good to me

…

On Sun, Dec 3, 2017, 11:00 AM rounddelta ***@***.***> wrote: Well, fileByLabel also uses T.isInfixOf. Should I create the exact version for this, too? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1459 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADBB4ZlHQKRg9db-XnxTQAGFGYwNn5Eks5s8mMogaJpZM4QzP8h> .

MaxGabriel · 2017-12-03T17:05:39Z

yesod-test/Yesod/Test.hs

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


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

Edit: unless this is meant to be exported?

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

MaxGabriel · 2017-12-03T17:08:10Z

yesod-test/Yesod/Test.hs


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


Is this meant to be exported? It seems really useful

No, neither.

MaxGabriel · 2017-12-03T17:35:15Z

yesod-test/Yesod/Test.hs

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


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

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.

MaxGabriel · 2017-12-03T17:46:26Z

yesod-test/Yesod/Test.hs

 -- > </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:


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"

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 · 2017-12-04T07:52:11Z

@snoyberg Should I create another PR?

snoyberg · 2017-12-04T07:55:01Z

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

pythonissam · 2017-12-04T08:15:32Z

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

MaxGabriel · 2017-12-17T14:12:16Z

yesod-test/Yesod/Test.hs

 -- >   <label>Please submit an image <input type="file" name="f1"> </label>
 -- > </form>
+--
+-- Warning: There exists the same issue of 'byLabel'. Please use 'fileByLabelExact' instead.


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

MaxGabriel · 2017-12-17T14:19:11Z

I made one small comment. Otherwise LGTM!

pythonissam · 2017-12-18T04:36:30Z

@MaxGabriel Noted with thanks!

pythonissam · 2017-12-18T05:11:49Z

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

MaxGabriel · 2017-12-18T15:46:23Z

@pythonissam what’s the other bug in byLabel?

pythonissam · 2017-12-18T22:44:38Z

@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 · 2017-12-18T23:33:11Z

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 · 2017-12-24T04:37:22Z

@MaxGabriel @snoyberg

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

snoyberg

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

snoyberg · 2017-12-24T05:46:23Z

yesod-test/Yesod/Test.hs

 type HasCallStack = (() :: Constraint)
 #endif

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


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

It sounds good

pythonissam · 2017-12-27T00:49:17Z

@snoyberg Is this okay?

snoyberg · 2017-12-27T11:02:47Z

Looks great, thanks!

MaxGabriel · 2017-12-27T15:18:41Z

Thanks @pyhtonissam!

pythonissam · 2017-12-27T21:39:55Z

@snoyberg @MaxGabriel Yay! Thank you, too!

Fix Haddock syntax error and test failures introduced by #1459

Decamark added 8 commits November 26, 2017 04:45

Add a failure test case for byLabel

cab78b6

Simply, create the exact version of byLabel

80aa45c

new function genericNameFromLabel which abstracts the match methods

70ec8c6

Remove the original nameFromLabel

8693c72

Improve the document

57e25ea

Improve the function name

8f21830

Add version info

db87b76

Version bump

e5c04a0

Update the Changelog

699203f

snoyberg approved these changes Dec 3, 2017

View reviewed changes

MaxGabriel reviewed Dec 3, 2017

View reviewed changes

Improve comments

baf30d0

fileByLabelExact: Created the exact version of fileByLabel

31903c3

MaxGabriel requested changes Dec 17, 2017

View reviewed changes

Small fix of haddock

8cf44ff

Insert deprecated pragmas for byLabel and fileByLabel

8e52b49

MaxGabriel approved these changes Dec 19, 2017

View reviewed changes

snoyberg reviewed Dec 24, 2017

View reviewed changes

Add PR link into the deprecated pragmas

daa953e

snoyberg approved these changes Dec 27, 2017

View reviewed changes

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

MaxGabriel added a commit that referenced this pull request Dec 30, 2017

Fix Haddock syntax error and test failures introduced by #1459

6ba44b0

MaxGabriel added a commit that referenced this pull request Dec 30, 2017

Fix Haddock syntax error and test failures introduced by #1459

7799cb1

MaxGabriel added a commit that referenced this pull request Dec 30, 2017

Fix Haddock syntax error and test failures introduced by #1459

51bdb66

snoyberg added a commit that referenced this pull request Dec 30, 2017

Merge pull request #1473 from yesodweb/fix1459

f9a87f4

Fix Haddock syntax error and test failures introduced by #1459

Conversation

pythonissam commented Dec 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pythonissam commented Dec 2, 2017

Uh oh!

pythonissam commented Dec 3, 2017

Uh oh!

snoyberg commented Dec 3, 2017 via email

Uh oh!

MaxGabriel Dec 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pythonissam commented Dec 4, 2017

Uh oh!

snoyberg commented Dec 4, 2017

Uh oh!

pythonissam commented Dec 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGabriel commented Dec 17, 2017

Uh oh!

pythonissam commented Dec 18, 2017

Uh oh!

pythonissam commented Dec 18, 2017

Uh oh!

MaxGabriel commented Dec 18, 2017

Uh oh!

pythonissam commented Dec 18, 2017

Uh oh!

pythonissam commented Dec 18, 2017

Uh oh!

pythonissam commented Dec 24, 2017

Uh oh!

snoyberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pythonissam Dec 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pythonissam commented Dec 27, 2017

Uh oh!

snoyberg commented Dec 27, 2017

Uh oh!

MaxGabriel commented Dec 27, 2017

Uh oh!

pythonissam commented Dec 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pythonissam commented Dec 2, 2017 •

edited

Loading

MaxGabriel Dec 3, 2017 •

edited

Loading

pythonissam Dec 27, 2017 •

edited

Loading