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

[Bug 1182904] Gather plugin version/filename in AAQ #2593

Closed
wants to merge 2 commits into from

Conversation

philipp-sumo
Copy link
Contributor

No description provided.

strtemp = navigator.plugins[i].name;
if (navigator.plugins[i].version) strtemp += ' ' + navigator.plugins[i].version;
if (navigator.plugins[i].filename) strtemp += ' (' + navigator.plugins[i].filename + ')';
d = strtemp.replace(/<[^>]+>/ig,'');
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Pretty sure there should be a space between the comma and the first quote.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that part is copied over from the existing code, but i could change that of course if you like...

@willkg
Copy link
Member

willkg commented Jul 12, 2015

Nit: The commit message doesn't conform to our guidelines. That's an issue since it won't work with our tools very well.

http://kitsune.readthedocs.org/en/latest/conventions.html#git-commit-messages

Can you write tests for this? Amongst other things, it'll help document the expectation of the shape of things you're parsing out for future maintainers.

@philipp-sumo philipp-sumo changed the title [bug1182904] gather plugin version and filename during aaq [Bug 1182904] Gather plugin version/filename in AAQ Jul 12, 2015
@philipp-sumo
Copy link
Contributor Author

hi will, i did change the commit message accoridingly but i'm sorry that i can't write a test for this change, since i'm not knowledgeable enough for that...

@safwanrahman
Copy link
Contributor

I think I can write test for this! I will try to write a test for this!
@willkg Can you please let me know, what the tests should do?

@willkg
Copy link
Member

willkg commented Jul 13, 2015

The tests should verify the code is working as intended. What kind of strings are being manipulated here? What kind of objects are being operated on? What possible error situations can occur?

@mythmon
Copy link
Contributor

mythmon commented Jul 13, 2015

You can add tests for JS code in kitsune/sumo/static/sumo/js/tests.

@safwanrahman
Copy link
Contributor

Sorry, have no Idea about Javascript tests! 😞

@mythmon
Copy link
Contributor

mythmon commented Jul 21, 2015

I don't want to merge this without tests. Unfortunately, the state of our JS tests aren't great, so this might be hard to accomplish. For now, I'm going to close this to clean things up. Let me know if you want to work on tests, and I can help you out.

@mythmon mythmon closed this Jul 21, 2015
@philipp-sumo
Copy link
Contributor Author

i've asked around amongst contributors but noone of us will be able to write js tests for this patch - is this something that sumodev could do, since the change in the pull-request itself would be most useful in the support forum?

@philipp-sumo philipp-sumo deleted the patch-10 branch April 16, 2020 14:54
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.

None yet

4 participants