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

Pulling in the magic strings from an online source #54

Merged
merged 6 commits into from
Jul 6, 2017

Conversation

noelmarkham
Copy link
Collaborator

This parses the data in the "big list of naughty strings" repo and provides a generator.

Originally I was trying to use a macro to parse the lines into a list, and I realised I didn't really need to do that :-/ ... I think the rationale behind that was to only have that list parsed once ever - at library compile time - but it wasn't really worth the effort when it can be done so simply when the Magic class is first referenced, I think this is a fine tradeoff.

@juanpedromoreno @fedefernandez what do you think?

Plenty of issues:
  - The current form creates some interesting problems with the
  generated string (control chars etc)
  - Need to migrate this to an SBT task, using SBT IO for
  reading/writing the strings file
  - Properly reference the strings file, removing it from resources
@fedefernandez
Copy link
Contributor

I'm okay with this approach.
Do you think it would need to be defined as lazy?
On the other hand, I'm not sure if the magic file will be packaged inside the jar

@noelmarkham
Copy link
Collaborator Author

I don't think lazy is necessary - if anything that's going the opposite way to which I'd want to go (ideally I want the list parsed at compile time as this only ever needs to be done once).

As for in the Jar, I assumed it would be as it's in the resources directory, but this is something that I should definitely test. Thanks!

@noelmarkham
Copy link
Collaborator Author

I've manually tested this and it works fine in a fresh project, pulling in the published Jar. I've created issue #55 to do the work to automate this.

If you're happy can I get a LGTM and I'll merge this.

Thanks!

@fedefernandez
Copy link
Contributor

Should it be Scala 2.10 and 2.11 compatible? The method fromResource is only available from Scala 2.12

@noelmarkham
Copy link
Collaborator Author

Ah yes, this is masked by the multiple build issues I was planning to fix after this. I'm going to mark this PR as blocked until I've fixed #52

@codecov-io
Copy link

codecov-io commented Jul 6, 2017

Codecov Report

Merging #54 into master will decrease coverage by 0.74%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #54      +/-   ##
=========================================
- Coverage   98.34%   97.6%   -0.75%     
=========================================
  Files          10      10              
  Lines         121     125       +4     
  Branches        4       4              
=========================================
+ Hits          119     122       +3     
- Misses          2       3       +1
Impacted Files Coverage Δ
...ala/com/fortysevendeg/scalacheck/magic/Magic.scala 100% <100%> (ø) ⬆️
...ortysevendeg/scalacheck/datetime/GenDateTime.scala 80% <0%> (-20%) ⬇️

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 c6891f3...16d7376. Read the comment docs.

@noelmarkham
Copy link
Collaborator Author

@fedefernandez are you able to review now please? This is working across Scala 2.[10, 11, 12].

Thanks

Copy link
Contributor

@fedefernandez fedefernandez left a comment

Choose a reason for hiding this comment

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

LGTM!

@noelmarkham noelmarkham merged commit c7e3548 into master Jul 6, 2017
@noelmarkham noelmarkham deleted the nm-31-magic branch July 6, 2017 13:20
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.

4 participants