search

smhg / gettext-parser

Parse and compile gettext po and mo files, nothing more, nothing less
MIT License
158 stars 43 forks source link

v8.1.0 Update #86

Open erikyo opened 2 months ago

erikyo commented 2 months ago

this is the PR that contains all the changes listed in #82

There are no dramatic changes, however, we may decide to move to 8.1.0 (and not 8.0.1) because of the modified type system, which did not require immediate action, but it would be preferable to suggest to remove the "definitively typed" types (if someone is using ts). For that reason a changelog file update is nice, hopefully someone is using npm outdated

NOT ready to merge

smhg commented 2 months ago

@erikyo thank you for your efforts! If you want me to review these changes, please make it easy and separate things into separate PR's. Like i've been requesting. Please put all cosmetic changes (imports, directory structure, typos,...) into one PR. I'm (by now) very happy to quickly review that. What then remains is only a limited set of changes related to TS which is harder for me to look at and the separation makes that review too much easier.

erikyo commented 2 months ago

@smhg, I've organized the pull requests (PRs) in the following manner: each individual PR that has been merged into this branch encompasses all the 'one-shot' modifications made to the repository for the next version.

PR #89 addresses the correction of JSdocs and typos. PR #81 focuses on type checking. PRs #84 and #90 deal with the replacement of deprecated elements. PR #85 involves the relocation of all files into the 'src' directory. PRs #86 and #87 optimize performance and eliminate a dependency. For a detailed explanation, please refer to this link.

Although you can navigate to the link to inspect each PR individually and check their respective changes, I've been gradually merging them into the development branch to preemptively address potential conflicts during the merge process. However, if any critical issues arise, please bring them to my attention, and I'll promptly rectify them.

Consider this branch as a pre-publication quality control checkpoint. I've included you in this process so that you have the "last word" over it.

smhg commented 1 month ago

@erikyo maybe I don't have the git skills required to understand your approach/workflow. To pick just one of the changes: I expected to see a PR that applies renaming /lib to /src being applied to the code currently in master. Instead, if I look at #85 I see changes to tsconfig.json file and a types property in package.json. Both don't exist in master. It doesn't really matter this is applied in a development branch, right? It is not possible to consider these changes separately (now and in the future) because they seem to be applied after TS related changes? Additionally, you can argue the changes in that PR related to import statements should also rather be applied separately. But I'd be fine with one PR for all these 'meta' changes before anything related to TS. Does what I'm suggesting make any sense to you? Or just tell me to shut up :)

erikyo commented 1 month ago

I understand your concerns, and I appreciate your feedback. Let me clarify my approach and workflow regarding the changes I've proposed.

The reason why you didn't see those changes in the master branch, is because I prefer to keep changes in a branch concurrent with the master branch. This has a simple reason... users that needs to download (or check the code) from the github repository should do so with the code from the latest release and not the code in development (which may not be stable). At this stage, i think it's better to keep these modifications on the dev branch, and this approach ensures that we can maintain a stable master branch until all planned modifications are finalized.

At the moment I'm still missing a set of modifications (#89 which is now a draft) because I'd like @johnhooks to check it too, that guy is really good at this and his contribute would be really helpful

About typescript: The current changes primarily focus on stabilizing and updating the repository, rather than introducing TypeScript-related updates. The TypeScript compiler (tsc) is currently utilized solely for generating types and compiling JavaScript. This is why you'll notice allowJs: true in the tsconfig.json.

In summary, I intend to delay merging any changes to the master branch until I've completed the entire set of modifications I've outlined. This allows us to maintain a clear separation between development and stable code (if you want to try the development version, however, just checkout the development instead of the master and you will be on top of all the new updates).

I hope this clarifies my approach, but I welcome any further discussion or suggestions you may have.

about the changes in that PR related to import statements

Yes this you are right, this kind of change could be done directly on the master, and not the kind of changes to put on the development branch stack. I will try to solve this as soon as possible!

johnhooks commented 1 month ago

I've read through the previous comments for this PR. I agree with @smhg, it would be best to create separate PRs for the following:

I also don’t see a reason to keep the main branch in sync with releases. That is the purpose of git tags and GitHub releases. Though it all depends on the workflow preferred by the maintainer.

erikyo commented 1 month ago

why manually create a type declaration, rather than use one generated by tsc?

Currently, there are 400 errors in types, and the generated output doesn't match our expectations. While your suggestion would be ideal, attempting to generate the file entirely now yields unexpected results.

image

👆 and since many types aren't defined they becomes "any" (which basically means not typed)

To address this issue fully, it necessitates either making very radical changes (or submitting several PRs). This partly explains the purpose of the dev branch (Release Branch Strategy). The rationale behind it is that the required changes are too extensive to 'fix everything' at once, yet an interim version cannot be published to the master branch.

johnhooks commented 1 month ago

In my opinion, types aren't currently published by the package,so it doesn't make sense to copy the Definitely Typed ones. Those are established and what people are used to. It would be best to wait and publish the actual types when they are ready.

I wouldn't copy the types from Definitely Typed, but enable types with very low strictness, and start improving the situation in increments. The types can be incomplete and unpublished (by unpublished, I mean not adding the "types" property to the package.json until they are ready). This would not require a full overhaul of the package in one large step. A build step can still be used to convert ts to js files. Flip them all to ts as they are and accept they will not pass a strict type check, or any type check, but then it can be worked at in digestible chunks.

erikyo commented 1 month ago

Sorry John, maybe I didn't provide enough context. The types from Definitely Typed are for version 4.0.4 and not for the current 8.0.0, and they are indeed very generic, sometimes incorrect (like in the case of comments where they are all marked as required when they're not) and they only cover the exported functions in the index. When I mentioned using the same types, I was referring to the naming convention chosen for the translation block, whether it's "GetTextTranslation" or "GetTextTranslations".

So, #89 isn't a copy of Definitely Typed, but it's obviously very similar since the functions and types are the same (as I mentioned earlier, even the names). However, they are correct, which isn't the case for Definitely Typed.

I'm taking steps forward: in #81, I started generating types (with some issues because index.js and index.d.ts were in the root), then in #85, I moved all the files into src (which resolved the problem), etc. Making it strictly typed requires changing or adding a lot of code, unfortunately not all JSDoc comments were correct, and this results in many lines to be changed, and consequently, if not one giant PR, then many small PRs (which is what I'm attempting to do).

johnhooks commented 1 month ago

Thanks @erikyo for the explanation, I'd would still lean toward waiting to publish them until they are "correct" and generated by properly typed code. IMHO

@smhg Could we please enable discussions for this repository? I think that could be a better place to hash out the details of git strategy, rather than in a PR or Issue

erikyo commented 1 month ago

@johnhooks After a bit of tweaking yesterday I managed to reduce the errors to 280, so I don't think the result is that far off.

you should try the #89 using "checkJs": true in tsconfig you would have a clearer situation 😉

smhg commented 1 month ago

@johnhooks I've started one discussion topic. Hopefully that does the trick. Please tell me if this isn't set up properly.