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

refactor #19

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

refactor #19

wants to merge 18 commits into from

Conversation

mangkoran
Copy link
Contributor

@mangkoran mangkoran commented Jan 27, 2024

WIP. I decided to do some refactor of the codebase so it looks more neat (IMO). Apparently it will solve #1 (comment) #9, #10 but not yet for now.

Current state/commit is on parity with master (no failed compilation).

Please let me know if you have any suggestions.

@mangkoran mangkoran marked this pull request as ready for review January 28, 2024 09:52
@mangkoran
Copy link
Contributor Author

mangkoran commented Jan 28, 2024

Alright I think it's now ready for review. No issue so far on my end. I think we need to update the JSON schema too as we made some fields optional, but I will revisit it later. Please let me know if you have any suggestions.

Friendly mention @jskherman @kyawzazaw @phamson02

@mangkoran
Copy link
Contributor Author

I also have an idea to split input files/YAML for sensitive data such as phone number, email, location etc. So we could track the other input file while keeping the sensitive data locally. What do you think about it?

cv.typ Outdated
})

for hi in p.highlights {
[- #hi]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this part should be reverted back to using eval(). This breaks the use of markup like bold or italics when present in the YAML file.

Suggested change
[- #hi]
for hi in p.highlights{
[- #eval(hi, mode: "markup")]
}

cv.typ Outdated
Comment on lines 231 to 233
for h in org.highlights {
[- #h]
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar problem here as well as above...

Suggested change
for h in org.highlights {
[- #h]
}
for hi in org.highlights {
[- #eval(hi, mode: "markup")]
}

cv.typ Outdated
Comment on lines 262 to 218
for h in project.highlights {
[- #h]
}
Copy link
Owner

@jskherman jskherman Feb 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for h in project.highlights {
[- #h]
}
for hi in project.highlights {
[- #eval(hi, mode: "markup")]
}

cv.typ Outdated
Comment on lines 288 to 290
for h in award.highlights {
[- #h]
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for h in award.highlights {
[- #h]
}
for hi in award.highlights {
[- #eval(hi, mode: "markup")]
}

cv.typ Outdated
Comment on lines 356 to 347
for skill in skills {
dot_list(skill.category, skill.skills)
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To reduce confusion, might I suggest to use the variable name group, skill-group, or skill_group?

Suggested change
for skill in skills {
dot_list(skill.category, skill.skills)
}
for skill_group in skills {
dot_list(skill_group.category, skill_group.skills)
}

cv.typ Outdated
Comment on lines 372 to 374
let foo = ()
foo.push(ref.reference)
dot_list(ref.name, foo, url: ref.url)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I ask if foo here is needed? Could it instead be an array created directly?

Suggested change
let foo = ()
foo.push(ref.reference)
dot_list(ref.name, foo, url: ref.url)
dot_list(ref.name, (ref.reference,), url: ref.url)

@jskherman
Copy link
Owner

I also have an idea to split input files/YAML for sensitive data such as phone number, email, location etc. So we could track the other input file while keeping the sensitive data locally. What do you think about it?

Hm, technically you could consider all of the data in a résumé or CV sensitive data. Also, it might get a bit too unwieldy to have the data scattered in different YAML files. To illustrate, my current use case involves using different YAML files slightly altered from a main YAML file (used in the CV) to create one/two-page résumés that are tailored to a specific job description or role. For me, having one YAML file per document makes it easier to diff these customized "versions".

@mangkoran
Copy link
Contributor Author

Hi @jskherman I apologize for the late reply as I am busy with with IRL. Originally this PR should be opened as a draft PR as it still need some rework. And thank you for leaving suggestions.

However, I'm planning to do some additional refactor to comply with Typst's template format. I think I will still use this PR for that.

@mangkoran mangkoran marked this pull request as draft March 30, 2024 13:53
@mangkoran
Copy link
Contributor Author

mangkoran commented Apr 10, 2024

Suggestions committed. Could you please recheck whether it's what you requested?

Meanwhile I will continue to update the project structure to match template format.

In most packages (e.g. tablex) functions are named after snake_case and
variables are named after kebab-case
By removing checks, we enforce the usage of some variables that are
deemed to be mandatory.

This commit also remove section data checks. So if user don't include
the respective section data, it implies the user don't want to include
the data and they need to exclude the respective section function call
in the template in order to compile.
@mangkoran mangkoran marked this pull request as ready for review April 12, 2024 05:50
After second check, it seems all of function and variable use kebab-case
In my opinion, targeting master branch for a schema would be an issue if
we introduce a breaking changes to the schema. By using local schema, it
ensures that the current schema is in line with current cv.typ version.
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

2 participants