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

Allow multiple formats of association links #9

Closed
wants to merge 1 commit into from

Conversation

vpArth
Copy link

@vpArth vpArth commented Mar 17, 2017

Followed formats are allowed for now:

  • /questions/123/question-title
  • /q/123/234
  • /a/321/234

Followed formats are allowed for now:

- /questions/123/question-title
- /q/123/234
- /a/321/234
function questionId(uri) {
var id = /\/(\d+)\//.exec(uri)
return id == null ? -1 : id[1];
function questionId(uri, site) {
Copy link
Owner

Choose a reason for hiding this comment

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

It looks like this function is called with uri that contains "stackoverflow.com/*". At the same time, the "site" contains a link to the current site (which means "ru.stackoverflow.com" for example). It seems that it should not work. Could you please check it?

Copy link
Author

Choose a reason for hiding this comment

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

site is a questionId argument.

questionId function has to be used 2 times in this script.

  • In checkPage(document.baseURI)
    with questionId('ru.stackoverflow.com/...', 'ru.stackoverflow.com') call
  • In getSEQuestionId({comment, linkToSE})
    with questionId('stackoverflow.com/...', 'stackoverflow')

Where is nothing wrong.

Copy link
Author

Choose a reason for hiding this comment

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

But, site argument value can be parsed from uri content and looks like redundant.

I was adding it just because we already parse it in getSEQuestionId

Copy link
Owner

Choose a reason for hiding this comment

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

It seems so.

@vpArth
Copy link
Author

vpArth commented May 14, 2021

Close PR because it is fairly outdated

@vpArth vpArth closed this May 14, 2021
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

2 participants