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

One-Stop Config File for Code Portfolio #2161

Open
sopa301 opened this issue Mar 18, 2024 · 14 comments
Open

One-Stop Config File for Code Portfolio #2161

sopa301 opened this issue Mar 18, 2024 · 14 comments

Comments

@sopa301
Copy link
Contributor

sopa301 commented Mar 18, 2024

What feature(s) would you like to see in RepoSense

Is the feature request related to a problem?

Currently, the format for configuring the one-stop config file repo-config.csv is difficult for inexperienced users to use. Additionally, it is not friendly for those trying to create a code portfolio.

If possible, describe the solution

Repurpose report-config.json into a one-stop config file in a file format that is more user-friendly. Possible formats include yml and xml. Additionally, include more configuration options (to be decided).

This means that the order of priority would be this new file, then repo-config, then the rest of the csv files.

Steps:

  1. Port the current functionality into yml and update the parser.
  2. Include more functionalities as necessary.
  3. Update documentation.

Additional context

Expanded version of item 2 of #2157

@damithc
Copy link
Collaborator

damithc commented Mar 18, 2024

I don't think we should alter the repo-config.csv file. It is optimized for situations where there is a large number of repos. What we need is a one-stop config file that can hold some basic config options of diverse nature, to cater for simpler cases. The specialized config files such as repo-config.csv need to remain for cases that need deeper configurations or to configure heavier datasets.

@sopa301
Copy link
Contributor Author

sopa301 commented Mar 18, 2024

Got it prof, I've updated the solution.

@damithc
Copy link
Collaborator

damithc commented Mar 18, 2024

@sopa301 a more suitable config file to repropose for this may be the report-config.json https://reposense.org/ug/configFiles.html#report-config-json

@damithc
Copy link
Collaborator

damithc commented Mar 27, 2024

Here's an example that makes sense from the user point of view, without being tied to JSON, YAML etc.:

title: John Doe's Code Dashboard
start: 2020-01-01
end: 2023-01-01
------
# John Doe's Code Dashboard

These are some of the work I've done in the past few years. Mostly for course projects, but some are pet projects.

====================================
repo:https://github.com/GERARDJM018/ip
branch: master
authorNames: johnDoe;John Doe;my home PC
------
# DukePro

This is a solo project I did for a course, using **Java**.

<img src="https://gerardjm018.github.io/ip/Ui.png"/>

[Product Home Page](https://johnDoe.github.io/ip/)

====================================
repo: https://github.com/AY2324S2-CS2103T-W08-1/tp
branch: master
authorNames: johnDoe;John Doe;my home PC
------
# Contacts Fun

This is a team project I did for a course, using **Java, JavaFX**.

<img src="https://gerardjm018.github.io/ip/Ui.png"/>

[Product Home Page](https://johnDoe.github.io/ip/)
Notable PRs: [Adding auto-scroll feature](https://github.com/AY2324S2-CS2103T-W08-1/tp/pulls/34)

@damithc
Copy link
Collaborator

damithc commented Mar 28, 2024

Another thing: There is no strict need to force the entire file to be in one format. For example, if you look at the example above, you can split it first by ======... and then by ------ and each chunk you get after splitting is either fully YAML or fully Markdown which can then go into the matching parser. That is, the file can be a combination of different formats.
While combining formats is not ideal, it may still be better than trying use one format for the whole file.

@gok99
Copy link
Contributor

gok99 commented May 12, 2024

#2192 Introduces blurb.md for specifying blurbs for repositories. It seems reasonable to me to include blurb specification to the one-stop config since users are likely to use the config for portfolios. This however would cause complications with different sources of truth for blurbs. There are number of ways we could deal with this:

  1. Don't allow blurbs to be specified in the one-stop config
  2. Move specification of blurbs to the one-stop config only
  3. Have one config be of a higher priority for specification in case there are conflicts
  4. Explicitly override the .md config like we do in the CSVs
  5. Something else

Each have different pros/cons which perhaps we could discuss.

@ckcherry23
Copy link
Member

I think option 3 would work best, considering the rest of the information in the one-stop config file can also be specified in author-config.csv, group-config.csv and repo-config.csv, leading to such conflicts.

@gok99
Copy link
Contributor

gok99 commented May 12, 2024

This is true, we have the standalone config, CSVs and now the one-stop config all of which could conflict 😣. I think it might make sense even to find a way to deprecate the standalone config in place of the one-stop config. The interaction/priorities between the one-stop config and the CSVs could be the same as the interaction of the standalone config with the CSVs.

@ckcherry23
Copy link
Member

Yes, #2171 is related to this.

@gok99
Copy link
Contributor

gok99 commented May 12, 2024

@ckcherry23 Thanks for linking that issue! It makes sense to me to do that together with this major new config, which has a lot of overlap in functionality

@damithc
Copy link
Collaborator

damithc commented May 16, 2024

Yup, we can expedite removing the standalone config file. I'm of two minds as to doing it in the same PR though. Reasons:

  1. Mainly, to keep the size of this change manageable. Removing the standalone config file may not be trivial (e.g., it is also mentioned in other config files).
  2. It is a breaking change. So, it will require the next release to jump to v4.0. Too soon? This feature itself is not a breaking change.
  3. Technically, it is a different feature from this one. So, the two can be considered as two different changes.

@gok99
Copy link
Contributor

gok99 commented May 16, 2024

This feature itself is not a breaking change.

I personally feel introducing a third conflicting config, and ensuring that the logic for handling them (over handling precedence rules for only two) is done in a consistent way will be quite challenging and might cause some trouble when the logic is later removed when we remove the standalone config. Doing a direct swap will let us keep the existing precedence rules and logic between the standalone config and CSV.

That being said, I do agree that removing the standalone config and introducing a fully featured new config in the same PR is a bit too large in scope. Perhaps we can break them down into 2 steps:

  1. A first PR to replace the standalone config with yaml config of exactly the same features.
  2. Second PR to add the additional features not captured in the standalone config, and resolve additional conflicts between the CSVs and yaml config

Step 1 will be breaking change however, and will require a major version bump. If this is undesirable, we could introduce the new config with the highest precedence and ignore all other configs if it's used (even if overriding), to reduce the implementation complexity. Given that the one-stop config is pretty fully featured, this behavior might be ok.

@damithc
Copy link
Collaborator

damithc commented May 16, 2024

I personally feel introducing a third conflicting config, and ensuring that the logic for handling them (over handling precedence rules for only two) is done in a consistent way will be quite challenging and might cause some trouble when the logic is later removed when we remove the standalone config. Doing a direct swap will let us keep the existing precedence rules and logic between the standalone config and CSV.

That being said, I do agree that removing the standalone config and introducing a fully featured new config in the same PR is a bit too large in scope. Perhaps we can break them down into 2 steps:

  1. A first PR to replace the standalone config with yaml config of exactly the same features.
  2. Second PR to add the additional features not captured in the standalone config, and resolve additional conflicts between the CSVs and yaml config

That's workable as well. If we are starting from scratch, this is probably what we should do. But as we've already done a PR for the one-stop config file, the choice is not that clear. So, I'll leave you all to decide if it is easier to add the 'remove standalone config' to the current PR, or switch to the above 2-step approach (or some other strategy).

Step 1 will be breaking change however, and will require a major version bump. If this is undesirable, we could introduce the new config with the highest precedence and ignore all other configs if it's used (even if overriding), to reduce the implementation complexity. Given that the one-stop config is pretty fully featured, this behavior might be ok.

Bumping the major version is fine, if needed. There is no real cost of doing so.

@gok99
Copy link
Contributor

gok99 commented May 17, 2024

If I'm not mistaken, while #2192 introduces the new one-stop config and corresponding fields, not all of them are integrated into the analysis. It seems like currently, it replaces just the report-config.json and the report title is now taken from one-stop config instead. It probably would not waste any work to have it do the same with the standalone config in a similar way (@asdfghjkxd could confirm if this is a correct assessment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

4 participants