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

Feature/lcs #1230

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Feature/lcs #1230

wants to merge 6 commits into from

Conversation

abhaysp95
Copy link

I'm adding the Longest Common Subsequence to sequence folder. But please do tell, if it's more appropriate to add this algorithm to dynamic programming folder

Copy link

Visit the preview URL for this PR (for commit abad93f):

https://cp-algorithms--preview-1230-ytie2m1h.web.app

(expires 2024-03-02T18:19:40.399280408Z)

@mhayter
Copy link
Contributor

mhayter commented Mar 5, 2024

Unfortunately, there are several grammatical errors like lack of agreement between subjects and verbs, spelling errors, and unusual sentence structure. There may be more substantive review points, but it feels moot to review at this point in the article's life.

As a point of style, I question if using lambdas make sense as I believe this is new to the website and is, perhaps, less accessible to those who don't know C++ well.

I'm not sure what the maintainers think.

@abhaysp95
Copy link
Author

Unfortunately, there are several grammatical errors like lack of agreement between subjects and verbs, spelling errors, and unusual sentence structure. There may be more substantive review points, but it feels moot to review at this point in the article's life.

As a point of style, I question if using lambdas make sense as I believe this is new to the website and is, perhaps, less accessible to those who don't know C++ well.

I'm not sure what the maintainers think.

It would be nice if you can point out some sentences as example were you saw grammatical errors, as english is not my first language. That way, I can try to improve it.

I'm not sure, what do you mean by "article's life" ?

I think lambdas were introduced in C++11 and many competitive sites are supporting >C++17 too. I don't think it should be a problem as it is also a core part of C++ now. But if there is a problem, please do tell

@mhayter
Copy link
Contributor

mhayter commented Mar 12, 2024

I'm very impressed that you were able to write what you've written in a non-native language. (I took 5 years of French and I can't really even speak it lol.) With that, if I knew I may have some issues writing, I'd consider using basic proofreading with some software. Check this out: https://quillbot.com/grammar-check

My comments aren't meant to be rude. I just think it would be common etiquette to make sure there aren't several grammar errors before issuing a pull request.

I think the review should be about substance, not about language.

My comment regarding the "article's life" is referring to the fact that the basics of grammar haven't been achieved to warrant a more thorough substantive review.

The "lambda issue" is a matter of taste. Like I said, I don't think they exist on the website yet and might not translate as well to other languages, but I'm not a maintainer so this isn't up to me.

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

3 participants