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

Enhance code lisibility #57

Merged
merged 3 commits into from
Apr 17, 2023

Conversation

abrisse
Copy link
Member

@abrisse abrisse commented Apr 17, 2023

Hi @gkellogg,

This PR (which is quite large) is a first step to enhance the code lisibility of the lib folder and reduce the technical debt. It includes the gem rubocop to help to detect the offenses to fix.

If that's OK for you I will do a next PR to enhance the tests.

@coveralls
Copy link

Coverage Status

Coverage: 82.906% (-1.5%) from 84.444% when pulling a663bea on PerfectMemory:feat/rubocop-ruby26 into 466be87 on ruby-rdf:develop.

@abrisse
Copy link
Member Author

abrisse commented Apr 17, 2023

I added a commit that declares some constants to avoid useless string allocations.

@gkellogg
Copy link
Member

Not sure why code coverage keeps going down on these PRs.

I've been ambivalent to Rubocop, as I don't always agree with the value judgements on code representation. While there may be some "code debt" in the changes (greater use of constants, for example), the main motivation for doing this is to enable more people to work on the code, which I'm all in favor of, as my time is limited and work on specifications has reduce the amount of time I have to maintain and/or enhance the large number of gems contributing to Ruby Linked Data.

@gkellogg gkellogg merged commit a73b483 into ruby-rdf:develop Apr 17, 2023
10 of 11 checks passed
@gkellogg
Copy link
Member

Feel free to commit to a a branch of the main repo in the future, which can make collaboration on PRs easier.

@abrisse abrisse deleted the feat/rubocop-ruby26 branch April 17, 2023 16:11
@abrisse
Copy link
Member Author

abrisse commented Apr 17, 2023

@gkellogg: I usually prefer to create a distinct feature branch for each feature I work on to keep them small. But in this case I can indeed create a unique feature branch "feat/optimization" on the fork repository if you prefer, that makes sense.

For the code coverage decrease, I guess it's because some parts of the code that are not run during the CI tests have been modified by my commit (from one line to several lines).

Thanks for the quick merge. I would love to be able to be help you on these ruby RDF gems, but it's quite hard because of all the knowledge required (the W3C specs & algorithms) which is a gate keeper to fully understand the code. I don't know what would be the first step, if you have any recommendation I would appreciate it.

@gkellogg
Copy link
Member

No, a feature branch on the main repo is the way to go.

parts of the code can be confusing, but no more confusing than the spec. The test suite is pretty comprehensive, but you might want to download it locally.

I wasn’t too knowledgeable about the specs when I started either, and we can always discuss the merits of changes in comments. There are some cool features other implementations have added for convenience, and there may be more that can be done to work better with Ruby idioms.

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