Skip to content

Feature/lcs #1230

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

Closed
wants to merge 6 commits into from
Closed

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
Contributor

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.

@adamant-pwn adamant-pwn deleted the branch cp-algorithms:main October 14, 2024 18:52
@adamant-pwn adamant-pwn reopened this Oct 14, 2024
@adamant-pwn adamant-pwn changed the base branch from master to main October 14, 2024 19:20
@mhayter
Copy link
Contributor

mhayter commented Apr 11, 2025

I'm closing this as it's extremely unlikely to be updated properly.

@mhayter mhayter closed this Apr 11, 2025
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.

3 participants