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

Implement initial parser #1

Merged
merged 16 commits into from Nov 29, 2016
Merged

Implement initial parser #1

merged 16 commits into from Nov 29, 2016

Conversation

doomspork
Copy link
Member

Currently a work-in-progress.

Copy link
Contributor

@ybur-yug ybur-yug left a comment

Choose a reason for hiding this comment

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

Only minor observations/thoughts LGTM overall to merge.

opts = [strategy: :one_for_one, name: UserAgentParser.Supervisor]
Supervisor.start_link(children, opts)
end

def parse(user_agent) do
Copy link
Contributor

Choose a reason for hiding this comment

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

For stuuff in this I prefer to only keep application configuration logic, but thats just a nitpick

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a helper, the real work is happening elsewhere. I personally think these make for a clean public API with UserAgentPaser.parse("...") vs something like UserAgentParser.Parser.parse("...").


def parse!(user_agent) do
{user_agents, os, devices} = Storage.list

Copy link
Contributor

Choose a reason for hiding this comment

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

pointless newline?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not done 😝

safe =
user_agent
|> String.trim

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a preferred style for the newline here? I feel like its made clear that the return is happening due to the lack of indentation. Don't have a strong opinion, just curious.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I follow, what do you mean @ybur-yug?

@doomspork
Copy link
Member Author

@ybur-yug don't get too caught up in code that looks unfinished — it is 😀 . I'd love to get your feedback on the structure of things as it's coming together.

@veverkap
Copy link

WHERE IS NAG?

@doomspork
Copy link
Member Author

doomspork commented Nov 16, 2016

Damn good question @veverkap! I need to add the config for credo and get it wired up. About to dig in now

@ybur-yug
Copy link
Contributor

@doomspork my B, misunderstood when you initially told me to peek so I just was being thorough. It all looks rad! Ill keep my eyes on it

@doomspork
Copy link
Member Author

No worries @ybur-yug, I wasn't clear 😀

device_replacement: 'Generic Feature Phone'
brand_replacement: 'Generic'
model_replacement: 'Feature Phone'

Copy link
Member Author

Choose a reason for hiding this comment

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

Extra whitespace.

@doomspork
Copy link
Member Author

Alrighty @veverkap & @ybur-yug, give 'er a go, should be done! Feedback welcomed.

@@ -1,19 +1,38 @@
defmodule UserAgentParser do
@moduledoc """
Copy link
Member Author

Choose a reason for hiding this comment

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

Sometimes coming up with these docs feels like a chore 😛

@doomspork
Copy link
Member Author

doomspork commented Nov 19, 2016

So unfortunately I just discovered: https://github.com/romul/uap-elixir. I'm not a huge fan of his implementation so I'm going to keep this but will require a repo/app name change, suggestions?

Spitballin':

  • ex_agent
  • ex_user_agent
  • ex_uaparser
  • ex_uap
  • ua_parser

@doomspork
Copy link
Member Author

I have found others! Still not sure I want to trash this work though...

@doomspork
Copy link
Member Author

Ping @ybur-yug

@ybur-yug
Copy link
Contributor

I like ua_parser. Simple and explanatory, and avoids the overused ex prefix

@doomspork
Copy link
Member Author

Sounds good @ybur-yug. I can make the changes to the project or if you want to get in some commits you can.

@ybur-yug
Copy link
Contributor

I can do it later today

@doomspork
Copy link
Member Author

I'll add you as a contributor @ybur-yug 👍

@doomspork doomspork merged commit 18d6cd5 into master Nov 29, 2016
@doomspork doomspork deleted the proposal branch November 29, 2016 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants