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

[WIP] Added html and markdown url to documentation_link #1451

Closed
wants to merge 5 commits into from
Closed

[WIP] Added html and markdown url to documentation_link #1451

wants to merge 5 commits into from

Conversation

vzamanillo
Copy link

The html and markdown url should be useful to visit the documentation in a browser or to parse the markdown and extract information about the rule (with some linters, etc...).

The html and markdown url should be useful to visit the documentation in a browser or to parse the markdown and extract information about the rule (with some  linters, etc...).
@vzamanillo vzamanillo changed the title Added html and markdown url to documentation_link [WIP] Added html and markdown url to documentation_link Feb 11, 2019
@vzamanillo
Copy link
Author

vzamanillo commented Feb 11, 2019

Thoughts about how to solve the ABC size metric warning? I was thinking in split the method, but should not work because there will be too many methods.

@mvz
Copy link
Collaborator

mvz commented Feb 12, 2019

Hi @vzamanillo, thanks for your contribution. I have a question and some remarks:

  • Do you have a concrete example of what you would like a linter to do with the markdown URL?
  • The 'raw' URL you used seems to be a github-internal URL that may change. The html page contains a link to the raw content that linters could follow. Alternatively, just changing 'blob' to 'raw' in the given URL gives the URL for that link.
  • Your proposed change is a breaking change. Let's see if we can accommodate your use case with a non-breaking change.

@mvz
Copy link
Collaborator

mvz commented Feb 12, 2019

(Don't worry about the ABC size metric for now.)

@vzamanillo
Copy link
Author

vzamanillo commented Feb 12, 2019

Hi Matijs,

Do you have a concrete example of what you would like a linter to do with
the `markdown` URL?

I have not my branch for linter-reek published yet but the intention is the same as linter-rubocop does, to show the rule description on linter marker tooltip, you can check the getMarkDown source code.

captura de pantalla de 2019-02-12 11-45-01

The 'raw' URL you used seems to be a github-internal URL that may change. The html page
contains a link to the raw content that linters could follow. Alternatively,
just changing 'blob' to 'raw' in the given URL gives the URL for that link.

This is what my actual implementation does, replace the URL parts to get the raw content URL.

Your proposed change is a breaking change. Let's see if we can accommodate your use
case with a non-breaking change.

Thanks, let me know about anything I can do.

@mvz
Copy link
Collaborator

mvz commented Feb 12, 2019

This is what my actual implementation does, replace the URL parts to get the raw content URL.

I'm not following. In this PR you replace almost the entire URL? My point is that your linter can take the URL that Reek already provides and replace blob by raw to get the data you want. Maybe I'm misunderstanding what you are trying to say here?

The screen shot for linter-rubocop you posted shows one problem with your approach: The documentation linked to was never created for non-human consumption, so you may get all kinds of artifacts in the output. There is no guarantee Reek's documentation will keep the structure it has currently.

@vzamanillo
Copy link
Author

This is what my actual implementation does, replace the URL parts to get the raw content URL.

I'm not following. In this PR you replace almost the entire URL? My point is that your linter can take the URL that Reek already provides and replace blob by raw to get the data you want. Maybe I'm misunderstanding what you are trying to say here?

Sorry, I was talking about my pending linter-reek implementation who's consume the raw markdown data, not about this PR.

The screen shot for linter-rubocop you posted shows one problem with your approach: The documentation linked to was never created for non-human consumption, so you may get all kinds of artifacts in the output. There is no guarantee Reek's documentation will keep the structure it has currently.

My purpose was to use a pattern in the documentation to get a brief description of the reek warning, for example, all reek documentation written in markdown has an "Introduction" section that could be used, it could be interesting.

@mvz
Copy link
Collaborator

mvz commented Feb 12, 2019

Sorry, I was talking about my pending linter-reek implementation who's consume the raw markdown data, not about this PR.

Ok, that explains it. I think that's actually a good approach for what you're trying to do.

all reek documentation written in markdown has an "Introduction" section that could be used

I'm afraid there's no guarantee it will stay that way.

My advice would be to put a link in the tooltip (if that's possible), that will open the page in the browser when clicked. It's beneficial for users of Reek to read the full docs for each smell.

@vzamanillo
Copy link
Author

Sorry, I was talking about my pending linter-reek implementation who's consume the raw markdown data, not about this PR.

Ok, that explains it. I think that's actually a good approach for what you're trying to do.

all reek documentation written in markdown has an "Introduction" section that could be used

I'm afraid there's no guarantee it will stay that way.

I get it.

My advice would be to put a link in the tooltip (if that's possible), that will open the page in the browser when clicked. It's beneficial for users of Reek to read the full docs for each smell.

This is what my linter-reek implementation is doing actually.

Thanks!

@vzamanillo
Copy link
Author

You can check this linter-reek PR in case you are curious to see the implementation that I mentioned earlier.

@vzamanillo vzamanillo closed this Feb 12, 2019
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.

2 participants