Skip to content

fix to #544 #547

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jun 10, 2019
Merged

fix to #544 #547

merged 6 commits into from
Jun 10, 2019

Conversation

@mfrasca
Copy link
Contributor

@mfrasca mfrasca commented May 21, 2019 •
edited
Loading

I'm opening this PR as work in progress. the underlying idea is better visibility of the solving process for issue #544 .

mfrasca and others added 3 commits May 21, 2019 07:55
@mfrasca
adding (failing) tests
c2dc6fb 
this is meant as a start for the patch fixing applegrew#544
@mfrasca
adding tests, please review: are these the desired values?
440fa0d
@mfrasca
this matches the tests, but I'm sure it needs to be refined.
d0ce0c9 
I've included some of the print statements that helped me follow the process
--- I used more, near a few of the `__init__` methods involved.  you don't
want to produce `logging` info, or do you?  I still miss the point where the
final writing into `self.attrs` happens, and I'm not so sure what other
attributes need the same treatment, if any.  anyway, I really would rather
intervene on the final writing into `self.attrs`, in particular if we
collect more attributes than just this one.
@mfrasca
Copy link
Contributor Author

mfrasca commented May 21, 2019

this patch triggers 4 failures, all after a simulated click. three of them fail because the tests assert different results on different widgets, but the result is now Please enter 2 or more characters, which is possibly the correct behaviour.

mfrasca and others added 2 commits May 21, 2019 11:54
@mfrasca
attempt fixing tests on travis.
5e26d82 
the thing is that here they didn't fail before, and don't fail now,
so this is mostly a shot in the dark.
@codingjoe
Allow overriding defaults per widget type
082801e
@codingjoe
Copy link
Collaborator

codingjoe commented May 23, 2019

@mfrasca I added one commit myself to change how the defaults are set. Would you mind reviewing it?

@codecov-io
Copy link

codecov-io commented May 23, 2019

Codecov Report

Merging #547 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #547   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines           8      8           
=====================================
  Hits            8      8

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2bb6f3a...082801e. Read the comment docs.

@codecov-io
Copy link

codecov-io commented May 23, 2019 •
edited
Loading

Codecov Report

Merging #547 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #547   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines           8      8           
=====================================
  Hits            8      8

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2bb6f3a...c0dd48a. Read the comment docs.

@mfrasca
Copy link
Contributor Author

mfrasca commented May 23, 2019

looks good to me. about testing, do you think you have all cases covered? I do miss some technical documentation, like the role of the parameters, or hints to "future me" because in a couple of weeks I will know nothing of this code. let me add, I'm quite happy about the process we followed. 👍

whenever I am back to using these widgets for the M2M, I'll review the docs and suggest additions if I miss anything.

@codingjoe codingjoe merged commit c15de46 into applegrew:master Jun 10, 2019
@marojenka
Copy link

marojenka commented Aug 14, 2019

Hi. Just as a remark. Enabling default value for data-minimum-input-length changed default behaviour so some ppl might be confused by this (I was a bit) as it was made in patch release.
Happy to have it regardless.

@mfrasca
Copy link
Contributor Author

mfrasca commented Aug 14, 2019

Hi. Just as a remark. Enabling default value for data-minimum-input-length changed default behaviour so some ppl might be confused by this (I was a bit) as it was made in patch release.
Happy to have it regardless.

may I suggest … do you think you can provide the unit test cases you are interested in?
as to guarantee that the behaviour stays frozen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mfrasca @codingjoe @codecov-io @marojenka